Consolidate authentication code

VERIFIED FIXED in 0.9

Status

Calendar
General
--
enhancement
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: dbo, Assigned: dbo)

Tracking

unspecified

Details

(Whiteboard: [gdata-0.5])

Attachments

(2 attachments)

(Assignee)

Description

10 years ago
Gdata and wcap both have similar prompting code to get the user's credentials; could be consolidated.
(Assignee)

Comment 1

10 years ago
Created attachment 321753 [details] [diff] [review]
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
Assignee: nobody → daniel.boelzle
Status: NEW → ASSIGNED
Attachment #321753 - Flags: review?(philipp)
(Assignee)

Comment 2

10 years ago
Created attachment 321755 [details] [diff] [review]
packages-static
Attachment #321755 - Flags: review?(philipp)
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.
Attachment #321753 - Flags: review?(philipp) → review+
Attachment #321755 - Flags: review?(philipp) → review+
(Assignee)

Comment 4

10 years ago
Corrected nits and checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Summary: Consolidate auth prompts → Consolidate authentication code
Target Milestone: --- → 0.9

Comment 5

10 years ago
Checked via mxr.mozilla.org -> VERIFIED
Status: RESOLVED → VERIFIED
Whiteboard: [gdata-cvs]
Whiteboard: [gdata-cvs] → [gdata-0.5]
You need to log in before you can comment on or make changes to this bug.