434735 - Consolidate authentication code

Status: VERIFIED FIXED

(Calendar :: General, enhancement)

Description

[Daniel Boelzle [:dbo]]

Gdata and wcap both have similar prompting code to get the user's credentials; could be consolidated.

Comment 1

[Daniel Boelzle [:dbo]]

Attachment: patch

- consolidates gdata/wcap auth code
- moves calUtils' calAuthPrompt to calAuthUtils.js
- enhances gdata's password mgr code and moves it to calAuthPrompt.js
- wcap: prepares to disable a calendar once the password prompt has been canceled
misc:
 - slight enhancement of doQueryInterface
 - removes ensureIID, changes all occurrences to use doQueryInterface

Comment 2

[Daniel Boelzle [:dbo]]

Attachment: packages-static

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 321753 [details] [diff] [review]
patch

>+    prompt: function capP(aDialogTitle, aText, aPasswordRealm, aSavePassword,
>+                          aDefaultText, aResult) {
>+        return this.mPrompter.prompt(aDialogTitle, aText, aPasswordRealm,
>+                                     aSavePassword, aDefaultText, aResult);
Either wrap all parameters or none. I find this hard to read, personally. Goes for both function parameters and prompt() parameters


>+            while (pwenum.hasMoreElements()) {
>+                try {
>+                    var pass = pwenum.getNext().QueryInterface(Components.interfaces.nsIPassword);
>+                    if (pass.host == passwordRealm) {
>+                         // found it!
>+                         username = pass.user;
>+                         password = pass.password;
>+                         found = true;
>+                    }
>+                } catch (ex) {
>+                    found = true;
>+                    break;
Why is the password found when an error occurrs?


>+        return doQueryInterface(this, calMgrCalendarObserver.prototype, aIID,
>+                                [Components.interfaces.nsIWindowMediatorListener, Components.interfaces.calIObserver]);
Wrap line


>+            WARN("nsIInterfaceRequestor requesting invalid interface: " + Components.interfacesByID[aIID]);
>+            throw e;
Nice, I like :-)
> function getFormattedString(aBundleName, aStringName, aFormatArgs, aComponent) {
>+    return calGetString(aBundleName, aStringName, aFormatArgs, aComponent || "gdata-provider");
> }
Also great :-) With js1.8 or 1.9 I can probably make this look even smoother (binding arguments to functions).

> function getCalendarCredentials(aCalendarName,
>                                 aUsername,
>                                 aPassword,
>                                 aSavePassword) {
>+    return calGetCredentials(getFormattedString("gdata", "loginDialogTitle"),
>+                             aCalendarName,
>+                             aUsername,
>+                             aPassword,
>+                             aSavePassword);
> }
I think I don't use this function too often, calGetCredentials could probably be used directly.

> function passwordManagerSave(aUsername, aPassword) {
>+    // xxx todo: fallen, please check: hostname changes on branch from aUsername to chrome-url
>+    calPasswordManagerSave(aUsername, aPassword,
>+                           "chrome://gdata-provider/" + encodeURIComponent(aUsername),
>+                           "Google Calendar");
> }
Same goes for this function.
About the todo comment: this will mean that all users will have to re-enter their password, right? I think the chrome uri thing is just a guideline and I could possibly get away with using a free-form string.



> function passwordManagerGet(aUsername, aPassword) {
>+    // xxx todo: fallen, please check: hostname changes on branch from aUsername to chrome-url
>+    // xxx todo: fallen check realm changes from "Google Calendar Login" to "Google Calendar"
>+    return calPasswordManagerGet(aUsername, aPassword,
>+                                 "chrome://gdata-provider/" + encodeURIComponent(aUsername),
>+                                 "Google Calendar");
> }
Same goes for this function, regarding both comments.
I can live with the change, realm string is unimportant.

> function passwordManagerRemove(aUsername) {
>+    // xxx todo: fallen, please check: hostname changes on branch from aUsername to chrome-url
>+    // xxx todo: fallen check realm changes from "Google Calendar Login" to "Google Calendar"
>+    return calPasswordManagerRemove(aUsername,
>+                                    "chrome://gdata-provider/" + encodeURIComponent(aUsername),
>+                                    "Google Calendar");
> }
Same comments

>                                     QueryInterface: function fbInterval_QueryInterface(iid) {
>-                                        ensureIID([calIFreeBusyInterval, nsISupports], iid);
>+                                        return doQueryInterface(this, null, iid, [calIFreeBusyInterval]);
>                                         return this;
One return is enough


r+ for code, I haven't tested for gdata though. Will do so soon.

Comment 4

[Daniel Boelzle [:dbo]]

Corrected nits and checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.

Comment 5

[Andreas Treumann]

Checked via mxr.mozilla.org -> VERIFIED