Merge lp:~nataliabidart/ubuntu-sso-client/add-api-doc into lp:ubuntu-sso-client

[Natalia Bidart]

Documented CredentialsManagement interface (LP: #673054).

[Stuart Langridge (sil)]

Notes:

"They all return information to the caller through signals, since all the operations (either against the keyring, or against the SSO webservice) are blocking"; possibly append here ", and so the Ubuntu One SSO API is entirely asynchronous."

"NOTE: formerly, the {{{ApplicationCredentials}}} interface was implemented under the {{{/credentials}}} obecjt path. That interface is deprecated and should not be used." It's deprecated, but does it still work? Do we have a plan for removing it in the future? What are the deficits of using it?

"Look for credentials on the user's keyring for "app_name". Currently, second parameter is not used and an empty dict should be passed." What happens if you don't pass the empty dict? If this breaks, then the docs need to say "an empty dict MUST be passed".

What happens if an app calls register or login, U1 pops up the signin/signup dialog, and the user kills the dialog or it crashes? (Note: this is not about the user cancelling the dialog; that will fire AuthorizationDenied as documented.) Is any signal generated in that case? (CredentialsError?)

Since it is critically important that two apps do not share the same app_name (or they'll get confused over which signals are meant for them), I'd suggest calling out in the documentation that app_name must be unique to your app.

What happens if a user runs the same app twice? Won't they both have the same app_name?

'"error_dict" is a dictionary containing at least 2 fields' -- why at least 2? What else might be there?

Corrections:

DBus -> D-Bus (numerous places)

that implements the freedesktop secrets specififcation -> specification

That object implements the {{{com.ubuntu.sso.CredentialsManagemenet}}} interface -> CredentialsManagement

the API that ubuntu-sso-client exposes thru -> through

ubuntu-one-client DBus API -> ubuntuone-client, surely?

all of these methods return immediatly -> immediately

{{{/credentials}}} obecjt path -> object

If exisitng credentials are not found -> existing

Emitted when the credenntials for "app_name" -> credentials

will be returned to the called through the {{{CredentialsFound}}} signal -> will be returned to the caller

review: Needs Fixing

[Natalia Bidart (nataliabidart)]

> Notes:
>
> "They all return information to the caller through signals, since all the
> operations (either against the keyring, or against the SSO webservice) are
> blocking"; possibly append here ", and so the Ubuntu One SSO API is entirely
> asynchronous."

Fixed.

> "NOTE: formerly, the {{{ApplicationCredentials}}} interface was implemented
> under the {{{/credentials}}} obecjt path. That interface is deprecated and
> should not be used." It's deprecated, but does it still work? Do we have a
> plan for removing it in the future? What are the deficits of using it?

Fixed (added "Nevertheless, current applications using this interface will be able to do so until the Ubuntu 11.04 release inclusive, since it won't be removed it until 11.10").

> "Look for credentials on the user's keyring for "app_name". Currently, second
> parameter is not used and an empty dict should be passed." What happens if you
> don't pass the empty dict? If this breaks, then the docs need to say "an empty
> dict MUST be passed".

In dbus, all parameters are mandatory. Fixed.

> What happens if an app calls register or login, U1 pops up the signin/signup
> dialog, and the user kills the dialog or it crashes? (Note: this is not about
> the user cancelling the dialog; that will fire AuthorizationDenied as
> documented.) Is any signal generated in that case? (CredentialsError?)

No signal is sent in this case, since the dbus service can't track this scenario...

> Since it is critically important that two apps do not share the same app_name
> (or they'll get confused over which signals are meant for them), I'd suggest
> calling out in the documentation that app_name must be unique to your app.

Clarification added.

> What happens if a user runs the same app twice? Won't they both have the same
> app_name?

Yeah, and I see no further issue with that... do you?

> '"error_dict" is a dictionary containing at least 2 fields' -- why at least 2?
> What else might be there?

Maybe, in a future. That's why we use dictionaries as parameters, to be able to change the parametes without breaking existent client apps.

> Corrections:
>
> DBus -> D-Bus (numerous places)

All fixed.

> that implements the freedesktop secrets specififcation -> specification

Fixed.

> That object implements the {{{com.ubuntu.sso.CredentialsManagemenet}}}
> interface -> CredentialsManagement

Fixed.

> the API that ubuntu-sso-client exposes thru -> through

Fixed!

> ubuntu-one-client DBus API -> ubuntuone-client, surely?

ubuntu-sso-client (fixed)

> all of these methods return immediatly -> immediately
>
> {{{/credentials}}} obecjt path -> object

Fixed and fixed.

> If exisitng credentials are not found -> existing
>
> Emitted when the credenntials for "app_name" -> credentials
>
> will be returned to the called through the {{{CredentialsFound}}} signal ->
> will be returned to the caller

All 3 fixed!

Thanks a lot for this feedback, it was very very good.

[lp:~nataliabidart/ubuntu-sso-client/add-api-doc]

662. By Natalia Bidart on 2010-12-07

Adding corrections from an awesome review.

[Stuart Langridge (sil)]

review: Approve

[dobey (dobey)]

9 +let other applications manage a set of credentials (in a per application basis)
10 +stored in the local keyring.

Should be "let other applications store and manage credentials in the local keyring."

17 +Since release v1.1.5,

Should be "Since version 1.1.5"

31 +interface, that has the methods and signals listed below. One important thing
32 +to note is that all of these methods return immediatley, and return no value.

"One important thing to note" is redundant. Should just be "All of these methods are asynchronous, and return immediately with no value. Signal handlers are required to handle results specially, where necessary." Also, "immediately" is misspelled in the diff (and corrected in my suggested change).

40 +should not be used. Nevertheless, current applications using this interface

"Nevertheless" here should probably be "However" instead.

46 +Any of this methods may emit the {{{CredentialsError}}} signal, with the
47 +caller's "app_name" as first parameter, and a string-string dict descibing the
48 +error. See below for details about this signal.

"Any of this" should be "Any of these" and "as first parameter" should be "as the first parameter" here. Also "describing" is misspelled as "descibing". I'd suggest perhaps using "dictionary with key and value both as strings" rather than "string-string dict" as well.

Actually, I'm seeing "as {only, parameter, result}" appearing quite often in the text. These should be "as the {only, parameter, result}" in all cases I think. There may be a couple where "a" is appropriate instead of "the," but for the most part, it looks like the should be used.

52 +Look for credentials on the user's keyring for "app_name". Currently, second
53 +parameter is not used and an empty dict must be passed.

There are a couple pretty much exact duplicates of this in the ext. Instead of "second parameter" here, you should use the name of the parameter in quotes. So in this specific case it would be 'Currently, "extra_params" is not used, and should be passed as an empty dict.' (At least, I am only suggesting the use of quotes here as you seem to use quotes for parameter names, when they are used in the methods. So I think we should preserve that style everywhere.)

(I haven't finished reviewing, but I'm going to set this needsfixing now, and continue reviewing as well.)

review: Needs Fixing

[Natalia Bidart (nataliabidart)]

> 9 +let other applications manage a set of credentials (in a per
> application basis)
> 10 +stored in the local keyring.
>
> Should be "let other applications store and manage credentials in the local
> keyring."

Changed.

> 17 +Since release v1.1.5,
>
> Should be "Since version 1.1.5"

Changed.

> 31 +interface, that has the methods and signals listed below. One
> important thing
> 32 +to note is that all of these methods return immediately, and return
> no value.
>
> "One important thing to note" is redundant. Should just be "All of these
> methods are asynchronous, and return immediately with no value. Signal
> handlers are required to handle results specially, where necessary." Also,
> "immediately" is misspelled in the diff (and corrected in my suggested
> change).

Suggestion added.

> 40 +should not be used. Nevertheless, current applications using this
> interface
>
> "Nevertheless" here should probably be "However" instead.

Changed.

> 46 +Any of this methods may emit the {{{CredentialsError}}} signal, with
> the
> 47 +caller's "app_name" as first parameter, and a string-string dict
> descibing the
> 48 +error. See below for details about this signal.
>
> "Any of this" should be "Any of these" and "as first parameter" should be "as
> the first parameter" here. Also "describing" is misspelled as "descibing". I'd
> suggest perhaps using "dictionary with key and value both as strings" rather
> than "string-string dict" as well.

Changed.

> Actually, I'm seeing "as {only, parameter, result}" appearing quite often in
> the text. These should be "as the {only, parameter, result}" in all cases I
> think. There may be a couple where "a" is appropriate instead of "the," but
> for the most part, it looks like the should be used.

All replaced.

> 52 +Look for credentials on the user's keyring for "app_name". Currently,
> second
> 53 +parameter is not used and an empty dict must be passed.
>
> There are a couple pretty much exact duplicates of this in the ext. Instead of
> "second parameter" here, you should use the name of the parameter in quotes.
> So in this specific case it would be 'Currently, "extra_params" is not used,
> and should be passed as an empty dict.' (At least, I am only suggesting the
> use of quotes here as you seem to use quotes for parameter names, when they
> are used in the methods. So I think we should preserve that style everywhere.)

Good catch, fixed.

> (I haven't finished reviewing, but I'm going to set this needsfixing now, and
> continue reviewing as well.)

Thanks!

[lp:~nataliabidart/ubuntu-sso-client/add-api-doc]

663. By Natalia Bidart on 2010-12-09

More fixes from the review.

664. By Natalia Bidart on 2010-12-09

Aspell check.

[dobey (dobey)]

review: Approve

=== modified file 'README'
--- README	2010-06-14 15:31:15 +0000
+++ README	2010-12-09 20:03:26 +0000
@@ -1,1 +1,213 @@
-This is the Ubuntu Single Sign-On client.
+= Ubuntu Single Sign-On Client =
+
+ubuntu-sso-client is a desktop application that provides a D-Bus interface to
+let other applications store and manage credentials in the local keyring.
+
+When requesting a set of credentials for a given application, if those
+credentials are not yet present on the user's keyring, the user is presented
+with a GUI to either register a new account, or log in using an existing
+account by means of the Ubuntu SSO web service.
+
+Since version 1.1.5, this service does not depend on the gnome-keyring
+service, but on any keyring service that implements the freedesktop secrets
+specification (see
+http://freedesktop.org/wiki/Specifications/secret-storage-spec for reference).
+
+== ubuntu-sso-client D-Bus API ==
+
+This section details the API that ubuntu-sso-client exposes through D-Bus. It
+is available under the:
+
+ * {{{com.ubuntu.sso}}} bus name, on the
+ * {{{/com/ubuntu/sso/credentials}}} object path.
+
+That object implements the {{{com.ubuntu.sso.CredentialsManagement}}}
+interface, that has the methods and signals listed below. All of these  methods
+are asynchronous, and return immediately with no value. Signal handlers are
+required to handle results specially, where necessary.
+
+They all return information to the caller through signals, since all the
+operations (either against the keyring, or against the SSO web service) are
+blocking, and so the Ubuntu SSO API is entirely asynchronous.
+
+NOTE: formerly, the {{{ApplicationCredentials}}} interface was implemented
+under the {{{/credentials}}} object path. That interface is deprecated and
+should not be used. However, current applications using this interface will be
+able to do so until the Ubuntu 11.04 release inclusive, since it won't be
+removed it until 11.10.
+
+=== com.ubuntu.sso.CredentialsManagement Methods ===
+
+Any of these methods may emit the {{{CredentialsError}}} signal, with the
+caller's "app_name" as the first parameter, and a dictionary with key and value
+both as strings describing the error. See below for details about this signal.
+
+==== find_credentials(String app_name, Dict of {String, String} extra_params) ====
+
+Look for credentials on the user's keyring for "app_name". Currently, the
+parameter "extra_params" is not used and an empty dict must be passed.
+
+If credentials were found for "app_name", the signal {{{CredentialsFound}}} is
+emitted with "app_name" and the corresponding credentials dict as the result.
+
+If credentials were not found, the signal {{{CredentialsNotFound}}} is emitted
+with "app_name" as the only parameter.
+
+==== clear_credentials(String app_name, Dict of {String, String} extra_params) ====
+
+Clear the credentials on the user's keyring for "app_name". Currently, the
+parameter "extra_params" is not used and an empty dict should be passed.
+
+This method always emits the signal {{{CredentialsCleared}}} with "app_name" as
+only parameter (except on error when {{{CredentialsError}}} is emitted).
+
+==== store_credentials(String app_name, Dict of {String, String} credentials) ====
+
+Store the given set of 'credentials' on the user's keyring, for "app_name".
+Second parameter 'credentials' must provide at least these 4 keys with valid
+values:
+
+ * 'token'
+ * 'token_secret'
+ * 'consumer_key'
+ * 'consumer_secret'
+
+If the credentials were successfully set for "app_name", the signal
+{{{CredentialsStored}}} is emitted with "app_name" as the only parameter.
+
+==== register(String app_name, Dict of {String, String} ui_settings) ====
+
+This method will try to fetch the credentials from the user's keyring for
+"app_name", and will emit the {{{CredentialsFound}}} signal if they are found.
+
+If existing credentials are not found, it will open a graphical dialog to let
+the user create a new account for "app_name", or login to an existing SSO
+account. Once the user is successfully registered or logged in, the newly
+acquired credentials for "app_name" will be stored in the keyring and will be
+returned to the caller through the {{{CredentialsFound}}} signal.
+
+ * 'app_name': will be displayed in the GUI header (so it should be human
+ readable), plus it will be used to find/build/clear tokens. Special attention
+ must be paid to this value since if it collides with some other app name,
+ return signals may be misleading because the identifier is the application
+ name.
+
+The second parameter 'ui_settings' can contain any of the following (none is
+required):
+
+ * 'help_text': an explanatory text for the end-users, will be shown below the
+ header in most of the registration process.
+
+ * 'ping_url': an url to open after successful token retrieval. If defined, the
+ email will be attached to the url and will be pinged with a OAuth-signed
+ request (this is usually useful for server application to keep track of new
+ tokens).
+
+ * 'tc_url': the link to a Terms and Conditions web page. If defined, a
+ checkbox to agree to the terms will be presented to the user, and will be
+ required to be checked to continue the registration. Also, a link to the terms
+ will be offered for browsing. If not defined, nothing will be shown.
+
+ * 'window_id': an X11 window identifier to be used for
+ gtk.gdk.set_transient_for, so the dialog is always shown above the specified
+ parent window. Usually it is str(!GtkWidget.window.xid) for GTK callers. If
+ not defined, no parent will be set.
+
+Additionally, this 'ui_settings' can provide 2 extra keys to define which UI
+module and UI class should be used in case the service needs to open a
+graphical interface to the end user. These keys are:
+
+ * 'ui_class': the name of a class that lives within 'ui_module' and that
+ accepts proper parameters (TODO: document parameters). An example of this
+ class can be seen at ubuntu_sso.gtk.gui.UbuntuSSOClientGUI.
+
+ * 'ui_module': a string pointing to a python module that holds 'ui_class' in
+ it, and is importable from the system PYTHONPATH.
+
+==== login(String app_name, Dict of {String, String} ui_settings) ====
+
+This method behaves like 'register' except that the graphical interface, if
+shown to the user, will offer login only functionality.
+
+So, it will try to fetch the credentials from the user's keyring for
+"app_name", and will emit the {{{CredentialsFound}}} signal if found. If
+existing credentials are not found, it will open a graphical dialog to let the
+user log in into an existing SSO account. Once the user is successfully logged
+in, the newly acquired credentials for "app_name" will be stored in the keyring
+and will be returned to the caller through the {{{CredentialsFound}}} signal.
+
+Options for parameters on 'ui_settings' match the one listed above for
+'register', except that the 'tc_url' will not be used even if defined.
+
+=== com.ubuntu.sso.CredentialsManagement Signals ===
+
+The applications listening for this signals should make sure the app_name sent
+as parameter for each signal matches the one they used for calling the methods
+defined in the previous section. If the app_name does not match, then the
+calling applications can safely assume those credentials or error messages are
+meant for another application and just discard them.
+
+==== CredentialsFound(String app_name, Dict of {String, String} credentials) ====
+
+The credentials are returned when this signal is emitted. It means that the
+credentials were either on the keyring, or that the login and/or register
+process was successful and a set of credentials has been stored on the keyring. 
+
+ * "app_name" is the application name as passed to the called method.
+
+ * "credentials" is a dictionary that contains at least four keys with the
+ token and consumer keys and secrets. The dictionary keys are named:
+
+  * 'token'
+  * 'token_secret'
+  * 'consumer_key'
+  * 'consumer_secret'
+
+==== CredentialsNotFound(String app_name) ====
+
+Emitted after 'find_credentials' was called and the credentials for "app_name"
+were not found in the user's keyring.
+
+ * "app_name" is the application name as passed to the called method.
+
+==== AuthorizationDenied(String app_name) ====
+
+Emitted when the user was presented with a graphical interface and s/he
+canceled the request for login and/or register by clicking on a 'Cancel'
+button before the process was completed.
+
+ * "app_name" is the application name as passed to the called method.
+
+==== CredentialsError(String app_name, Dict of {String, String} error_dict) ====
+
+This signal is raised when there is any problem getting, setting or clearing
+the credentials.
+
+ * "app_name" is the application name as passed to the called method.
+
+ * "error_dict" is a dictionary containing at least 2 fields:
+
+  * "error_message" a simple message that can be shown to the user explaining
+  the error (is not translated to any language).
+
+  * "detailed_error" is a string containing a Python stacktrace or something
+  similar to help the developers debug the problem. The application calling
+  ubuntu-sso-client may choose to show it to the user using a !GtkExpander, or
+  write it to a log file, etc.
+
+==== CredentialsCleared(String app_name) ====
+
+Emitted when the credentials for "app_name" were successfully removed from the
+user's keyring. Note that if an application requested the removal of
+non-existent credentials, this signals will be emitted if no error occurred.
+
+ * "app_name" is the application name as passed to the called method.
+
+==== CredentialsStored(String app_name) ====
+
+Emitted when the credentials passed as the parameter to 'store_credentials'
+were successfully stored.
+
+ * "app_name" is the application name as passed to the called method.
+
+== To be done: add API docs for accounts service ==