1544596 - Lightning: Allow multiple connections with different users to the same CalDAV server

Status: RESOLVED FIXED

(Calendar :: Internal Components, enhancement)

Description

[John Bieling (:TbSync)]

Attachment: property.patch

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/73.0.3683.103 Safari/537.36

Steps to reproduce:

Currently lightning does not allow to set up two or more calendars from the same CalDAV server authenticated with different users. This is caused by the following two issues:

1. The password lookup method uses the host part of the URI and picks the first matching username/password entry from the password manager. It is not possible to actually specify the desired user. This can be fixed by adding the username as a property to the calendar.

2. If issue 1 is fixed, lightning runs into a caching issue of the underlying network library, which is also just using the host part of the URI to access cookie and password caches. This can be fixed by using containers (see bug 1494955).

I want to fix both issues in this bug, as both issues must be fixed at the same time. However, as the patch will touch lots of different parts of lightning, I want to get the patch reviewed step by step.

For the start, I attached a patch that is just adding the required field to the UI and updates the new username property.

Comment 1

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

Comment on attachment 9058445 [details] [diff] [review]
property.patch

Review of attachment 9058445 [details] [diff] [review]:
-----------------------------------------------------------------

You might want to create a meta bug for the larger issue, and then have the patches in dependent bugs. We usually want to have one issue per bug.

This is a good start, but there are a few things to consider:
* The decision which providers support this shouldn't be in the properties dialog, but a property on the calendar.
* There needs to be some migration path for calendars that have not been set up with a username
* Are there any concerns about saving the username outside of the login manager? This might be personal information that shouldn't be saved outside of the login manager, e.g. when a master password is used.
* Keep in mind bug 306495 is going to change a lot on how the calendar creation dialog works. I've been meaning to upload the patches so someone else can finish them.
* The patch should do more than just add the property, because right now it doesn't actually do anything.

For the last point I'm concerned about the interactions between the login manager and the calendars. The code would need to prefer this username over the one set in the login manager.

Comment 2

[John Bieling (:TbSync)]

It is of course difficult to patch things with respect to other things which have not yet landed. :-)

One important thing however: TbSync is scheduled to become part of Thunderbird (that is my main agenda to impr UX) and if it is accepted it will drastically change how calendars will be set up. All that autodiscover stuff is done in TbSync (now avail already). The idea is, that lightning will just provide the raw CalDAV engine as it is doing now, but the manaegment of any(!) remote resource is handled by TbSync. That is still a long way, but I am working hard on that.

That is why my proposed changes to Lightning are only minimal.

Furthermore I had the impressions that a large all-in-patch would take too much time to review and lightning itself is very complicated (the UI part is overlaying itself multiple times) so I thought adding stuff step-by-step would be better. But I can of course add more or all parts of the patch.

The decision which providers support this shouldn't be in the properties dialog, but a property on the calendar.

I need to know that before the calendar is created. we can of course make that a global method and each provder (in its provider overlay) can add itself to the list (this would also remove redundancy). But since I did not knwo which JS file this global method should go in, I did it localy. But yes, this is exactly the discussion I was looking for.

There needs to be some migration path for calendars that have not been set up with a username

This will be handled by the later parts of my patch. But basically: If no username is provided, lightning is working as now and picks up the first match from the password manager based on the host. Only if there is a username, it picks the entry matching host + user.

Are there any concerns about saving the username outside of the login manager? This might be personal information that shouldn't be saved outside of the login manager, e.g. when a master password is used.

Well, I do not think a username is something to be concerned about. If I recall, the username of IMAP/SMTP/POP is stored in prefs as well. I will check.

The patch should do more than just add the property, because right now it doesn't actually do anything.

That is intentional. You have the full XPI / github from my email with the full picture. But I can add all of it to this patch. Shoudl I?

For the last point I'm concerned about the interactions between the login manager and the calendars. The code would need to prefer this username over the one set in the login manager.

Same as above. If you agree, I would like to work on the UI first and add a global method for providers to "enable" the username feature. Alternativly:Eeach provider will add its own username (for example caldav-username) and we do not handle that globally. But since there WAS already a username (vommented out) in the global XUL, i thought you want to do this globally.

Comment 3

[John Bieling (:TbSync)]

Attachment: username.patch

This patch now includes the changes to actually use the username to get the password, so you can see how that is used. As you can see, all my changes aim to be fully backward compatible.

For that to work, we need the username inside Prompt() and I was not able to get it from the passed aChannel. So I created an a Prompt() instance for each username and made the username a member. If it is possible to get the calendar property "username" from the aChannel, than this can be simplified, but I worked on this for a long time and could not find a better solution.

I also looked at the UI code to move the addition of the username field to the caldav-lightning-calendar-creation.XUL overlay but this would require a lot of work. Can you decide, whether you want to handle the hide/show of the username field globaly by adding a method to allow providers to add themselves to the list, or if the caldav provider do that on its own? This is only needed for the UI, all the other code is just checking if the username prop is not empty to decide,w ether to use it or not.

We

Comment 4

[John Bieling (:TbSync)]

Attachment: container.patch

And this is the full patch, which is actually adding container support. You already switched to the codebasePrincipal, so it is just adding the userContextId which must be unique per username.

Connections for userA @ server1 and userA @ server2 can exist in the same container, as the nsIHttpChannel is breaking them up via the different host. But userA @ server1 and userB @ server1 must be placed into different containers.

You could of course place every(!) connection into a different container, but I would think that is an overkill.

The function getContainerIdForUser is local to Lightning. We could of course also add a global function to TB which manages that, which takes any type of key and returns integers. We would then use "Lightning:<username>" as key to get out own set of containers.

Comment 5

[John Bieling (:TbSync)]

FYI: SMTP and IMAP usernames are stored in prefs, so I would not consider storing the username as a prop of the calendar a security issue. Objections?

Comment 6

[John Bieling (:TbSync)]

Fallen: I currently do not know how to proceed. Do you need further info from me? I think I answered all your questions and also added the full patch.

I think the next thing you need to decide is, if the username property should be a general property of calendars (as now) or a special extension to the caldav calendar. My personal impression is, that a general property can be implemented more simple but I will give it a try if you want to have that contained withing the caldav provider.

Comment 7

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

Have this in my queue to take a look at.

Comment 8

[John Bieling (:TbSync)]

I just learned, that it is possible to access the originAttributes from the channel:

aChannel.loadInfo.originAttributes.userContextId

However, it is not possible to add any custom properties to the originAttributes (like the username).

So my changes regarding Prompt() could be simplified by doing a reverse lookup of the userContextId<->username map. We would be back to one instance of Prompt() and would not need to pass the username into the constructor.

Just wanted to let you know.

Comment 9

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

Comment on attachment 9058577 [details] [diff] [review]
container.patch

Review of attachment 9058577 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks good. I'm fine having one container per username, what you write makes sense. Would be good to capture that in a code comment so it is clear. I'm not too set the number of instances, if there is a smooth solution that involves a reverse lookup that works for me as well, happy to take a look.

Would it be feasible to only allow the user to provide a username in the calendar creation dialog? I'm thinking this should be pretty much fixed afterwards, if the user wants to change their username they are likely accessing a whole different calendar, so the providers may be confused if the content changes.

For the code itself as this gets closer to review, I'm going to be asking for making use of the linter, you can do (cd comm; ../mach eslint calendar) to lint the calendar files. New functions should also have a jsdoc header, see examples in calendar/base/modules/utils where I was pretty complete with them, and if I haven't done it there since recently we're using type info there as well (e.g. `@param {nsIFoo} foo      The foo bar thing`)

I didn't find a simple way to get the username that didn't involve re-parsing auth headers, so I think storing that separately works fine.

I'm going to upload the patches to the promised bugs and cc you on them so you know what I am talking about.

::: calendar/base/modules/utils/calProviderUtils.jsm
@@ +26,5 @@
> +        let max = 2999;
> +
> +        //init
> +        if (!(this._containers)) {
> +            this._containers = [];

You could well use a Map() for this I believe, this will avoid the O(n) indexOf lookup.

@@ +29,5 @@
> +        if (!(this._containers)) {
> +            this._containers = [];
> +        }
> +
> +        //reset if adding an entry will exceed allowed range

No need to comment every block, I think this is the only comment you'll need for that function. Bonus points for starting with a capital letter :)

@@ +31,5 @@
> +        }
> +
> +        //reset if adding an entry will exceed allowed range
> +        if (this._containers.length > (max-min) && this._containers.indexOf(username) == -1) {
> +            for (let i=0; i < this._containers.length; i++) {

Do we need to clear all the containers here, or would it make sense to create a circular data structure where it will clear the oldest entry?

@@ +32,5 @@
> +
> +        //reset if adding an entry will exceed allowed range
> +        if (this._containers.length > (max-min) && this._containers.indexOf(username) == -1) {
> +            for (let i=0; i < this._containers.length; i++) {
> +                //Services.clearData.deleteDataFromOriginAttributesPattern({ userContextId: i + min });

I'd prefer use of the clearData service, any specific reason you chose the observer notification instead?

::: calendar/resources/content/calendarCreation.js
@@ +202,3 @@
>  
>      gCalendar.name = cal_name;
> +    gCalendar.setProperty("username", cal_username);

You'll only want to set and show this if the calendar supports it.

You can check `gCalendar.getProperty("capabilities.username.supported")` and add that property to the caldav calendar.

Comment 10

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

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #9)

@@ +31,5 @@

•    }
  

•    //reset if adding an entry will exceed allowed range
  

•    if (this._containers.length > (max-min) && this._containers.indexOf(username) == -1) {
  

•        for (let i=0; i < this._containers.length; i++) {
  

Do we need to clear all the containers here, or would it make sense to
create a circular data structure where it will clear the oldest entry?

I thought this was a fun exercise, so here you go:

class LimitedMap extends Map {
  constructor(iterable, maxsize=Infinity, onclear=()=>{}) {
    super(iterable);
    this.maxsize = maxsize;
    this.onclear = onclear;
    this.order = [];
  }

  get full() {
    return this.size == this.maxsize;
  }

  get first() {
    return this.get(this.order[0]);
  }

  set(key, value) {
    if (!this.has(key)) {
      if (this.full) {
        let first = this.order.shift();
        this.onclear(first, this.get(first));
        this.delete(first);
      }
      this.order.push(key);
    }

    return super.set(key, value);
  }
}

class ContainerMap extends LimitedMap {
  constructor(iterable, min, max) {
    super(iterable, (max - min), () => {
      /* Services.clearData.... */
    });
    this.base = min;
  }

  getContainerForUser(username) {
    if (this.has(username)) {
      return this.get(username);
    }

    let nextId = this.full ? this.first : this.base + this.size;
    this.set(username, nextId);

    return nextId;
  }
}

Comment 11

[John Bieling (:TbSync)]

Attachment: bug1544596.patch

1. eslint
   =========
   This patch has passed eslint, but it still has two warnigs, which it does not print and the help page does not provide any info, on how to display them :-(

John@R2D2 ~/Documents/Mozilla/source/comm
$ ../mach eslint calendar
? 0 problems (0 errors, 2 warnings)

2. capabilities.username.supported
   ==================================
   I am now using capabilities.username.supported and it works perfectly. I hope it is as you anticipated this.

3. ContainerMap
   ===============
   I am now using a map, slightly different from what you proposed and I added it directly to the cal object, so it can be accessed by

cal.containerMap.getUserContextIdForUsername(username)
cal.containerMap.getUsernameForUserContextId(userContextId)

What I would really like to do is to make that containerMap globaly available within Thunderbird. We have to make an announcement regarding this anyhow, because we need to make sure no one is using our range. So instead we could announce to use the global containerMap instead.

If you agree, I would also like to change the map and add an additional owner or prefix identifier. Something like this:

???.containerMap.getUserContext("lightning", "john.bieling")

Internally the map simply uses the concatenation of both strings as key. This would ensure, that no other consumer is using the same container by accident, just because it is using the same key.

4. Prompt()
   ==========

I am now using the revers lookup of the username based on the userContextId of the channel.

5. Saving Passwords
   ===================
   I also had to change to save password method, to honor the provided user.

What do you think?

Comment 12

[John Bieling (:TbSync)]

To simplify and maybe speed up the processing of this bug, I decouple the question about a general implementation of the userContextId map and just ask for review of this patch. If I have feedback from magnus on this matter, I can land a patch which switches to the general implementation.

Comment 13

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

Comment on attachment 9061010 [details] [diff] [review]
bug1544596.patch

Review of attachment 9061010 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, I've been blocking on this long enough, let's do it. A few comments and nits:

::: calendar/base/content/dialogs/calendar-properties-dialog.xul
@@ +66,5 @@
> +        <label value="&locationpage.username.label;"
> +              disable-with-calendar="true"
> +              control="calendar-username"/>
> +        <textbox id="calendar-username"
> +              disable-with-calendar="true"/>

Can you align the attributes here?

::: calendar/base/modules/calUtils.jsm
@@ +21,5 @@
> +
> +/**
> +* A map that handles userContextIds and usernames.
> +*/
> +class ContainerMap extends Map {

This is probably a good fit for calDataUtils.jsm, but since it is exclusively used for auth you could also put it in calAuthUtils.jsm

@@ +29,5 @@
> +     *
> +     * @param {number} min - The lower range limit of userContextIds to be used.
> +     * @param {number} max - The upper range limit of userContextIds to be used.
> +     */
> +    constructor(min=0, max=Math.pow(2, 32)-1, iterable) {

I'd prefer using a const at the top level and using it here and below.

@@ +491,5 @@
> +     *
> +     * @param {number} min - The lower range limit of userContextIds to be used.
> +     * @param {number} max - The upper range limit of userContextIds to be used.
> +     */
> +    containerMap: new ContainerMap(20000, 29999)

This on the other hand is probably best kept in calAuthUtils.

::: calendar/base/modules/utils/calAuthUtils.jsm
@@ +54,5 @@
>              let password;
>              let found = false;
>  
>              let logins = Services.logins.findLogins({}, aPasswordRealm.prePath, null, aPasswordRealm.realm);
> +            for (let i=0; i < logins.length && !found; i++) {

Can you make sure to run eslint? This will probably not pass operator spacing. Also, for..of would work here if you use `break`.

@@ +279,5 @@
>              let newLoginInfo = Cc["@mozilla.org/login-manager/loginInfo;1"]
>                                   .createInstance(Ci.nsILoginInfo);
>              newLoginInfo.init(origin, null, aRealm, aUsername, aPassword, "", "");
> +
> +            for (let i=0; i < logins.length; i++) {

for..of should work here

::: calendar/base/modules/utils/calProviderUtils.jsm
@@ +47,5 @@
> +        // userA @ server1 and userB @ server1 must be placed into different containers.
> +        // It is therefore sufficient to add an individual userContextId per username.
> +        // prepHttpChannel is always called with aNotificationCallbacks = this/self = calendar,
> +        // which allows us to access the username property of the calendar.
> +        if (aNotificationCallbacks.getProperty("capabilities.username.supported") === true) {

Can you QI aNotificationCallbacks to the right interface? It is luckily always a calendar, but in theory it just needs to adhere to nsIInterfaceRequestor.

Comment 14

[John Bieling (:TbSync)]

Attachment: bug1544596_v2.patch

I made all the proposed changes.

However, I am not able to actually test the patch, because lightning is broken in tip and I currently fail to build beta.

Could you look at the patch if it is as you like? As soon as I was able to test, I will request review and approval for beta. But since time is running out for Tb68b2, I would like to ask you to look at the patch now already. Thank you very much for your help.

Comment 15

[Jorg K (CEST = GMT+2)]

Beta try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6da2d73857b0b47b7d0522eb6f5ced01f6f09282

Comment 16

[John Bieling (:TbSync)]

Attachment: bug1544596_v3.patch

Thanks to the try build I did find two errors, caused by moving stuff around and not updating all the references. I fixed those locally in the lightning extension and after that everything is working as expected.

I updated the patch in the same way. It should be good now. Just to be on the very safe side, could jorgk do another try build with the updated patch?

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 9071860 [details] [diff] [review]
bug1544596_v3.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=464190ff01e727d00f7beff5d127b329bc05f02a

Please use NI for general info ;-)

Comment 18

[John Bieling (:TbSync)]

The new try build works like a charm!

Comment 19

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

Comment on attachment 9071860 [details] [diff] [review]
bug1544596_v3.patch

Review of attachment 9071860 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with these nits. Both are optional, when you are ready go ahead and set r+ and request uplift.

::: calendar/base/modules/utils/calAuthUtils.jsm
@@ -14,5 @@
>  // NOTE: This module should not be loaded directly, it is available when including
>  // calUtils.jsm under the cal.auth namespace.
>  
>  this.EXPORTED_SYMBOLS = ["calauth"]; /* exported calauth */
>  

constants like this are uppercase by convention. Maybe use MAX_CONTAINER_ID

@@ +48,5 @@
> +        this.max = Math.max(max, ContainerMapMaxSize);
> +        if (this.min > this.max) {
> +            throw new RangeError("[ContainerMap] The provided min value " +
> +              "(" + this.min + ") must not be greater than the provided " +
> +              "max value (" + this.max + ")");

You could consider using template strings here. `... ${min} must not be greater than ${this.max}`. You could also consider removing this check, developers should know what they are doing.

::: calendar/base/modules/utils/calProviderUtils.jsm
@@ +54,5 @@
> +            calendar = aNotificationCallbacks.QueryInterface(Ci.calICalendar);
> +        } catch (e) {
> +            // Failed to get the calendar, do not use containers.
> +            Cu.reportError(e);
> +        }

Did anything speak against using cal.wrapInstance? It seems you are doing the same thing here. https://searchfox.org/comm-central/source/calendar/base/modules/calUtils.jsm#363

Comment 20

[John Bieling (:TbSync)]

Attachment: bug1544596_v4.patch

I changed the const as proposed and now use cal.wrapInstance().

I kept the check for max > min because I actually limit max so a min-max-pair provided by the developer with max>min could end up being max<min. I also did not switch to string templates, because I could not get it working with multilines and correct indentation without adding spaces to the actual string.

Comment 21

[John Bieling (:TbSync)]

I do not know how to set "checkin-needed". I better stop playing around with stuff I do not know. Jörg? Help! This is ready to go!

Comment 22

[Jorg K (CEST = GMT+2)]

"checkin-needed" can be set in the keywords.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/18fc7a8887b9
Allow multiple connections with different users to the same CalDAV server. r=philipp

Comment 24

[Jorg K (CEST = GMT+2)]

Backout:
https://hg.mozilla.org/comm-central/rev/ae1624177e2cf555a9d1f35bcae4c2e6287545bc
Backed out changeset 18fc7a8887b9 (bug 1544596) for test failures in testLocalICS.js. a=backout

I explicitly ran all tests in my try pushes
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6da2d73857b0b47b7d0522eb6f5ced01f6f09282
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=464190ff01e727d00f7beff5d127b329bc05f02a

You, the reviewer and I should have noticed that testLocalICS.js consistently failed :-(

I confirmed this by running the test locally with
mozmake -C comm/calendar/test/mozmill SOLO_TEST=testLocalICS.js mozmill-one
from the object directory.

I had to change pref("security.allow_eval_with_system_principal", true); in mail/app/profile/all-thunderbird.js since for some reason I was running into an eval debug crash otherwise.

Comment 25

[John Bieling (:TbSync)]

Uff, that fail is caused by my patch? I do not use eval. I have no idea what to do now.

Comment 26

[John Bieling (:TbSync)]

It is a good thing, that we have those tests. I learned so much doing this patch. I will try to understand what is happening.

Comment 27

[Jorg K (CEST = GMT+2)]

No, the eval stuff just got in the way when running the test in debug mode. As the logs and the local runs show, the test is broken like so:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=251600311&repo=try-comm-central&lineNumber=2237

SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\_tests\mozmill\stage\testLocalICS.js | testLocalICS.js::testLocalICS
  EXCEPTION: items is undefined
    at: elementslib.jsm line 295
       _forChildren elementslib.jsm:295 21
       _byID elementslib.jsm:316 24
       reduceLookup elementslib.jsm:454 30
       Lookup.prototype.getNode elementslib.jsm:486 19
       MozMillController.prototype.type controller.jsm:398 59
       handleNewCalendarWizard test-calendar-utils.js:541 16
       testLocalICS/< testLocalICS.js:49 9
       Runner.prototype.wrapper frame.jsm:552 7
       startTest test-window-helpers.js:320 18
       openCalendarWizard calWindowUtils.jsm:26 16
       doCommand calendar-common-sets.js:370 28
       goDoCommand globalOverlay.js:81 18
       traceFunc test-window-helpers.js:1505 27
       oncommand messenger.xul:1 1
       MozMillController.prototype.click controller.jsm:511 13
       click controller.jsm:224 22
       testLocalICS testLocalICS.js:51 25
       Runner.prototype.wrapper frame.jsm:556 7
       Runner.prototype._runTestModule frame.jsm:626 14
       Runner.prototype.runTestModule frame.jsm:665 8
       Runner.prototype.runTestFile frame.jsm:525 8
       runTestFile frame.jsm:677 10
       Bridge.prototype._execFunction server.js:190 15
       Bridge.prototype.execFunction server.js:195 21
       Session.prototype.receive server.js:313 6
       AsyncRead.prototype.onDataAvailable server.js:78 16
  EXCEPTION: menuitem is null
    at: test-calendar-utils.js line 625
       menulistSelect test-calendar-utils.js:625 5
       setData test-item-editing-helpers.js:246 9
       testLocalICS/< testLocalICS.js:59 9
       invokeEventDialog test-calendar-utils.js:315 5
       testLocalICS testLocalICS.js:56 5
       Runner.prototype.wrapper frame.jsm:556 7
       Runner.prototype._runTestModule frame.jsm:626 14
       Runner.prototype.runTestModule frame.jsm:665 8
       Runner.prototype.runTestFile frame.jsm:525 8
       runTestFile frame.jsm:677 10
       Bridge.prototype._execFunction server.js:190 15
       Bridge.prototype.execFunction server.js:195 21
       Session.prototype.receive server.js:313 6
       AsyncRead.prototype.onDataAvailable server.js:78 16

Now you need to run the test locally and work out why your changes affect it and why it fails. Very wild guess: You're changing the UI in calendar-properties-dialog.xul and the test exercises that UI. Maybe it clicks in the wrong location now or expects some other menu order or some such.

Comment 28

[John Bieling (:TbSync)]

Attachment: bug1544596_v5.patch

It was the creation dialog. The test searched for the location field via its align attribute. I added a new username-row before taht location-row with the same align attribute -> Mozmill entered the url in the wrong field.

I now gave that row an id and let the test search for the location row by that id.

Comment 29

[John Bieling (:TbSync)]

The test passes locally now. Philipp gave r+ via IRC. Ready to go.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/99b37b22c1f1
Allow multiple connections with different users to the same CalDAV server. r=philipp DONTBUILD

Comment 31

[Jorg K (CEST = GMT+2)]

TB 68 beta 2 / Cal 7.0:
https://hg.mozilla.org/releases/comm-beta/rev/f220ad81591e9d3ed1b33956721552c7846cb326