Closed Bug 646865 Opened 9 years ago Closed 9 years ago

Passwords doc needs revising

Categories

(Add-on SDK Graveyard :: Documentation, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wbamberg, Assigned: wbamberg)

Details

Attachments

(1 file, 1 obsolete file)

See bug 642335 for details.
Drew's comments on the passwords doc, copied over from bug 642335, just for convenience:

****

We've been trending away from using terms like "application" and "host
application" in favor of "Firefox" for addon-kit.  Please change that.

Also, Firefox's password manager is routinely referred to as the "password
manager", not the "login manager" or the "login management system", so please
change that, too.

The doc uses three related terms, "password", "login", and "credential",
without ever explaining the distinctions between them.  If there are no
distinctions meaningful to the reader, the doc should pick one and stick with
it.  I don't think that's the case though, so there needs to be some discussion
about the terms, and the doc should then use them in a way consistent with that
discussion.

>-        // credentials is an array of all credentials with a given `realm`.
>+        // credentials is an array of all credentials
>+        // with a given `realm`.

While you're here, could you capitalize all these comments, since they're
complete sentences?

>-        // credentials is an array of all credentials associated with given url.
>+        // credentials is an array of all credentials
>+        // associated with the supplied url.

The other comments backtick-quote the properties they mention, so please be
consistent: either backtick-quote "url" here or don't backtick-quote any of the
others.

>-        // credentials is an array of all credentials associated with given url
>-        // and given realm for useres with given username. 
>+        // credentials is an array of all credentials
>+        // associated with given url, realm, and user name

Same here, and please end this comment with a period like the others.

>         credentials.forEach(function(credential) {

What is this "credential" object?  Is it the same or similar to the options
object passed to search() and store()?  The doc doesn't say.

>+An object containing fields associated with a credential being searched. It may
>+contain any combination of the following fields:

Please use the word "properties" instead of "fields", here and everywhere else.

>+* `username`
>+* `password`,
>+* `realm`
>+* `url`
>+* `usernameField`
>+* `passwordField`

"password" has a stray comma.

>+All these fields are described in detail under the `store` section. Any

The "store section" appears in the browser as "store(options)" -- maybe it
would be better to refer to it as "the `store` function below"?

>+<api name="store">
>+@function
> 
>-@prop  [onComplete] {function}
>-The callback function that is called once credential is stored.
>+This function is used to store credentials in the host
>+application's built-in login manager.
> 
>-@prop [onError] {function}
>-The callback function that is called if storing a credential failed. Function is
>-passed an `error` containing a reason of a failure.
>+It takes an `options` object as an
>+argument: this contains the containing fields needed to create a login
>+credential.

"this contains the containing fields"?

>-## Storing an add-on associated credential ##
>+** Storing a credential associated with an add-on**

This section is a little oblique: what I want to know as a consumer of this API
is how to store a credential associated with *my* add-on, not some arbitrary
add-on.  So could you talk about "your add-on" instead of add-ons in general?

>-Add-ons also may store credentials that are not associated with any web sites.
>-In such case `realm` string briefly denotes the login's purpose.
>+Add-ons may store credentials that are not associated with any web sites.
>+In this case the `realm` string briefly denotes the login's purpose.

"In this case, use the `realm` string to briefly denote the login's purpose."

>-## Storing a web page associated credential ##
>+** Storing a credential associated with a web page **
> 
>-Most sites use HTML form login based authentication. Following example stores
>-credentials for such a web site:
>+Most sites use HTML form login based authentication. The following example
>+stores a credential for such a web site:

"HTML form login based authentication" is a mouthful.  "HTML form-based
authentication"?

>-## Storing a site authentication login ##
>+** Storing a site authentication login **
> 
>-Some web sites use HTTP/FTP authentication mechanism and associated credentials
>+Some web sites use HTTP/FTP authentication. The associated credentials
> contain different fields:
> 
>     require("passwords").store({
>@@ -195,20 +153,80 @@ sends a reply such as:
>     Server: Apache/1.3.27
>     WWW-Authenticate: Basic realm="ExampleCo Login"
> 
>-If website does not sends `realm` string with `WWW-Authenticate` header then
>-`realm` property must be omitted.
>+If the website does not include the `realm` string in the `WWW-Authenticate`
>+header, then the `realm` property must be omitted.

"web site" and "website" are used inconsistently throughout this doc.  Please
consistently use one or the other.

This section is confusing.  It seems to expect me, the consumer of this API, to
examine HTTP responses.  How do I do that?  Do I have to do that to use this
API?  Or am I supposed to do that manually with some tool and then use that
info when writing my add-on?  Also, according to this example response, the
WWW-Authenticate header includes a "Basic realm" string, not a "`realm`
string".

>+Which properties are valid for `options` depends on the type of
>+authentication, but regardless of that there are two optional callback
>+`onComplete` and `onError` properties that may be passed to observe the
>+success or failure of the operation.

"... there are two optional callback properties, `onComplete` and `onError`,
..."

>+@param options {object}
>+An object containing fields associated with a credential being created and
>+stored. Some fields are necessary for one type of authentication and not
>+for another. Please see examples to find more details.

This paragraph is paraphrasing the paragraph above it.  The paragraph above
should simply replace this one.

>+@prop username {string}
>+The user name for the login.

Since the property's name is "username" and not "userName", s/user
name/username/.

>+@prop [url] {string}
>+The `url` to which the login applies, formatted as a URL (for example,
>+"http://www.site.com"). A port number (":123") may be appended.

"The url... formatted as a URL"?  WTF?  And why is the first "url" lowercased
and backticked?

>+_Please note: `http`, `https` and `ftp` URLs should not include path from the
>+full URL, it will be stripped out if included._

What does "should not include path from the full URL" mean?  These URLs should
not include paths at all?  Clear examples of what to do and what not to do,
separate from the prose, would be good.  Also, this sentence is a comma splice.

>+@prop [formSubmitURL] {string}
>+The URL a form-based login was submitted to.

This use of past tense here and elsewhere in the store() description is
confusing.  I'm storing a credential; I didn't submit anything anywhere, did I?
 I think this means you were supposed to have looked at how some web site's
authentication works and written down the form's URL or something.

>+Forms with no `action` attribute default to submitting to their origin URL, so
>+that should be stored here. (`formSubmitURL` should not include the path from
>+the full URL, it will be stripped out if included).

Same comment here as in the "Please note" sentence above.

>+@prop [realm] {string}
>+The HTTP Realm for which the login was requested.

Why is Realm capitalized here?

>+@prop [usernameField] {string}
>+The `name` attribute for the user name input in a form. Non-form logins
>+must omit this field.

s/user name/username/

>+@prop [onError] {function}
>+The callback function that is called if storing a credential failed. The
>+function is passed an `error` containing a reason of a failure.

"an `error`"?  What does that mean?  Why is it in backticks?  What kind of
object is it?

> <api name="remove">
> @function
>-@param options {object}
>-When removing a credentials the specified `options` object must exactly match
>-what was stored (including a `password`). For this reason it is recommended to
>-chain this function with `search` to make sure that only necessary matches are
>-removed.
> 
>-## Removing a credential ##
>+Remove a stored credential.

s/Remove/Removes/

>+@param options {object}
>+When removing a credential the specified `options` object must exactly match
>+what was stored (including a `password`). For this reason it is recommended to
>+chain this function with `search` to make sure that only necessary matches are
>+removed.

"necessary matches" is weird.  "stored credentials"?

"chain" is ambiguous.  It would be clearer to say that `search` should be used
beforehand to retrieve the target credential, and to point to the first example
in this section.
> 
> >-## Storing a site authentication login ##
> >+** Storing a site authentication login **
> > 
> >-Some web sites use HTTP/FTP authentication mechanism and associated credentials
> >+Some web sites use HTTP/FTP authentication. The associated credentials
> > contain different fields:
> > 
> >     require("passwords").store({
> >@@ -195,20 +153,80 @@ sends a reply such as:
> >     Server: Apache/1.3.27
> >     WWW-Authenticate: Basic realm="ExampleCo Login"
> > 
> >-If website does not sends `realm` string with `WWW-Authenticate` header then
> >-`realm` property must be omitted.
> >+If the website does not include the `realm` string in the `WWW-Authenticate`
> >+header, then the `realm` property must be omitted.
> 
> 
> This section is confusing.  It seems to expect me, the consumer of this API, to
> examine HTTP responses.  How do I do that?  Do I have to do that to use this
> API?  Or am I supposed to do that manually with some tool and then use that
> info when writing my add-on?

I don't know the answer to this. Myk, or Irakli, could you help with this? How do we expect a user of this API to get the HTTP authentication data?
Also: the doc describes the onComplete option to store() as optional, but if I omit it, I get an error like:

error: An exception occurred.
Traceback (most recent call last):
  File "resource://jid0-04cbe23veepc3of5vclk4ytwjw8-pw1-lib/main.js", line 1, in 
    require("passwords").store({
  File "resource://jid0-04cbe23veepc3of5vclk4ytwjw8-addon-kit-lib/passwords.js", line 73, in 
    onComplete(wrapped(options));
TypeError: onComplete is not a function

(the corresponding code is the first example under the store() function:

require("passwords").store({
  realm: "User Registration",
  username: "joe",
  password: "SeCrEt123",
});

)

Which is right, the code or the docs? Or am I confused?
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Target Milestone: --- → 1.0
The docs are right; that option should be optional.  I have filed bug 649361 on the problem.
Attached patch Revised passwords doc (obsolete) — Splinter Review
There are a couple of questions I'm still not sure about here, so I'm asking for feedback before asking Drew for a review.

1) This revision says that add-on's credentials are segregated from each other: so if I call `search` without any options, I'll get back my credentials, and credentials for web sites, but not credentials for other add-ons. I'm sure that's not the case right now, but I'm not certain whether it ought to be the case or not. Bug 646649 tracks this.

2) I've referred to `self.uri` in here, which is currently undocumented. I'm not sure whether we want it to be part of the supported APIs or not. If so, it should be documented in `self`: if not, I should change this.
Attachment #527345 - Flags: review?
Attachment #527345 - Flags: feedback?(myk)
Comment on attachment 527345 [details] [diff] [review]
Revised passwords doc

Well, given the latest on bug 646649, it looks as if the text in here is OK. I'm still not quite sure abut self.uri, but I'd like this to land for 1.0b5, so it would be great if you could review it anyway, Drew.
Attachment #527345 - Flags: review? → review?(adw)
Comment on attachment 527345 [details] [diff] [review]
Revised passwords doc

I haven't been following the passwords API so I'm not sure what's implemented and what's not, but let me say up front that this doc should document the API as it currently functions, not as it's supposed to function.  When follow-up API bugs are fixed, then the doc should be updated accordingly.

Will, before I r+, is that the case?

>+The `passwords` module allows add-ons to interact with Firefox's
>+[Password Manager](http://support.mozilla.com/en-US/kb/Remembering%20passwords)
>+to add, retrieve and remove stored _credentials_.
>+
>+A "credential" is the set of information a user supplies to authenticate

Typically a term is styled when it's used in the sentence that defines it.  So I think you should not style "credentials" in the first paragraph but style it in the second paragraph, where it's defined, either with italics or boldface.

>+herself to a service. Typically a credential consists of a username and a
>+password.

People usually say "authenticate with a service," not "to a service," but maybe that's an Americanism? :)

>+Using this module you can:
>+
>+1. **Search** for credentials which have been stored in the Password Manager.
>+   You can then use the credentials to access the service (for example, by
>+   logging into a web site).

"access the service"... what service?  "access their related service" would be clearer.

>+## Credentials ##
>+
>+All credentials in this API include a username and a password. Different sorts
>+of stored credentials include various additional properties, as outlined in
>+this section.

You transition from speaking about credentials in an abstract sense above to credential objects in this API here without explaining the latter.  It would be helpful to first say something like, "In this API, credentials are represented by objects.  All credential objects include `username` and `password` properties.  Different sorts..."  (Or more accurately I guess, "In this API, credentials are represented by sets of properties on objects," but that's cumbersome.)  (See also Myk's second paragraph in bug 642335 comment 3.)

>+You can use the Passwords API with three sorts of credentials:
>+
>+* Add-on credentials
>+* HTML form credentials
>+* HTTP authentication credentials

After reading the sections that follow, it's not clear to me if I'm expected to create these credential objects or if they'll be given to me from the API (or both).  Sometimes the text says "you should" and other times it says things like "this property contains," as if it will be filled in by someone else.  I can read the API reference section to find out more, but it would be helpful if the prose introduced me to how these credential objects are used.

>+### Add-on Credential ###
>+
>+These are associated with your add-on, rather than a particular web site.

comma not needed

>+<tr>
>+  <td>
>+    <code>realm</code>
>+  </td>
>+  <td>
>+    You can use this as a name for the credential, to distinguish
>+    it from any other credentials you've stored.

Is `realm` ever shown to the add-on's user (e.g., in Firefox's Passwords Manager UI)?  I seem to recall reading that in the original version of this doc, and looking at my own Password Manager I see for example "chrome://weave (Mozilla Services Password)", where "Mozilla Services Password" is the realm, I believe.   If so, that should be stated here.

>+### HTML Form Credential ###
>+
>+These credentials are used to authenticate the user to a web site using an
>+HTML form. They contain the following properties:

Ambiguous: is the authentication done using an HTML form, or is the web site using an HTML form?  Does this mean that if some web site I want to authenticate the user with uses HTML forms, I'm supposed to use this type of credential (as opposed to the "HTTP Authentication Credential")?

>+<tr>
>+  <td>
>+    <code>formSubmitURL</code>
>+  </td>
>+  <td>
>+    The value for the form's "action" attribute.

value for -> value of

>+<tr>
>+  <td>
>+    <code>usernameField</code>
>+  </td>
>+  <td>
>+    The value of the "name" attribute for the form's "username" field.
>+  </td>
>+</tr>
>+
>+<tr>
>+  <td>
>+    <code>passwordField</code>
>+  </td>
>+  <td>
>+    The value of the "name" attribute for the form's "password" field.

"username" and "password" probably shouldn't be quoted, since, unlike the "action" attribute also mentioned, there's no DOM element or piece of UI necessarily named either.

>+The corresponding values for the credential  (excluding username and password)
>+should be:
>+
>+<pre>
>+  url: "http://www.example.com",
>+  realm: "ExampleCo Login",

trailing comma

>+`options` may contain a function assigned to its `onError` property, which is
>+called if the function encounters an error. `onError` is passed the error as an
>+[nsIException](https://developer.mozilla.org/en/nsIException) object.

Oh man, really?  High-level Jetpack APIs should not be exposing nsIFoos.  This should be a standard JavaScript Error object.  Could you file a bug on fixing that?

>+@prop [username] {string}
>+The user name for the credential.

user name -> username

> @prop [realm] {string}
>-The HTTP Realm for which the login was requested. When an HTTP server sends a
>-401 result, the WWW-Authenticate header includes a realm to identify the
>-"protection space." See [RFC 2617](http://tools.ietf.org/html/rfc2617). If the
>-result did not include a realm, then option must be omitted. For logins
>-obtained from HTML forms, this field must be omitted. 
>-For add-on associated credentials this field briefly denotes the credentials
>-purpose (It is displayed as a description in the application's built-in login
>-management UI).
>+For HTTP Authentication credentials, the realm for which the credential was

"Authentication" in "HTTP Authentication credentials" is capitalized here (and a few other places), but before this it's not.  The doc should be consistent.

> @prop [usernameField] {string}
>-The `name` attribute for the user name input in a form. Non-form logins
>-must omit this field.
>+The `name` attribute for the user name input field in a form.

The `name` attribute for the user name -> The value of the `name` attribute of the username

> @prop [passwordField] {string}
>-The `name` attribute for the password input in a form. Non-form logins
>-must omit this field.
>+The `name` attribute for the password input field in a form.

The `name` attribute for -> The value of the `name` attribute of

>+@prop [usernameField] {string}
>+The `name` attribute for the username input in a form. Omitted for add-on

The `name` attribute for -> The value of the `name` attribute of

>+@prop [passwordField] {string}
>+The `name` attribute for the password input in a form. Omitted for add-on

The `name` attribute for -> The value of the `name` attribute of

>+@prop [usernameField] {string}
>+The `name` attribute for the username input in a form. Omitted for add-on

The `name` attribute for -> The value of the `name` attribute of

>+@prop [passwordField] {string}
>+The `name` attribute for the password input in a form. Omitted for add-on

The `name` attribute for -> The value of the `name` attribute of
(In reply to comment #7)
> Comment on attachment 527345 [details] [diff] [review]
> Revised passwords doc
> 
> I haven't been following the passwords API so I'm not sure what's implemented
> and what's not, but let me say up front that this doc should document the API
> as it currently functions, not as it's supposed to function.  When follow-up
> API bugs are fixed, then the doc should be updated accordingly.
> 
> Will, before I r+, is that the case?

In the previous patch: no. In this patch: yes.

I hope this addresses your other review comments.

> >+called if the function encounters an error. `onError` is passed the error as an
> >+[nsIException](https://developer.mozilla.org/en/nsIException) object.
> 
> Oh man, really?  High-level Jetpack APIs should not be exposing nsIFoos.  This
> should be a standard JavaScript Error object.  Could you file a bug on fixing
> that?

Bug 652988.
Attachment #527345 - Attachment is obsolete: true
Attachment #527345 - Flags: review?(adw)
Attachment #527345 - Flags: feedback?(myk)
Attachment #528465 - Flags: review?(adw)
Comment on attachment 528465 [details] [diff] [review]
Addressed review comments

>+## Credentials ##
>+
>+In this API, credentials are represented by objects.
>+
>+To use the `search` function you supply a set of search criteria and the
>+function returns a list of credential objects.
>+
>+To use the `store` function you create a credential object to be stored and
>+pass it in as an argument.
>+
>+To use the `remove` function you create a credential object exactly matching
>+the stored credential to be removed and pass that in as an argument.

At this point the doc hasn't explained what functions are available, so it's a little abrupt that it's telling me how to use each one.  I think I had something simpler in mind, like, "You create credential objects to pass into the API, and the API also returns credential objects to you.  The sections below explain both the properties you should define on credential objects and the properties you can expect on credential objects returned by the API."

>+All credential objects include username and password properties. Different

username -> `username`

password -> `password`

>+<tr>
>+  <td>
>+    <code>realm</code>
>+  </td>
>+  <td>
>+    <p>You can use this as a name for the credential, to distinguish
>+    it from any other credentials you've stored.</p>
>+    <p>The realm is displayed
>+    in Firefox's Password Manager, under "Site", in brackets after the URL.
>+    For example, if the realm for a credential is "User Registration", then
>+    its "Site" field will look something like:</p>
>+    <code>addon:jid0-01mBBFyu0ZAXCFuB1JYKooSTKIc(User Registration)</code>

I think there should be a space between the JID and "(User Registration)".  At least that's what these strings look like in my own Password Manager.

Thanks Will!
Attachment #528465 - Flags: review?(adw) → review+
Thanks Drew. Fixed by: https://github.com/mozilla/addon-sdk/commit/bdf7a1e3b4360859ecf94e10740b3a1eaf2dbf7d .
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.