1047667 - Unregister logged in user from the Loop server upon logout

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Matthew N. [:MattN]]

I believe this requires a deleting the push registration on the server.

Comment 1

[Adam Roach [:abr]]

(In reply to Matthew N. [:MattN] from comment #0)
> I believe this requires a deleting the push registration on the server.

Correct; see http://docs.services.mozilla.com/loop/apis.html#delete-registration

Comment 2

[Matthew N. [:MattN]]

Should we close the existing Web Socket Channel for Push after deleting the push registration? We would then get a new push URL and register it as a guest on the server. The remaining piece to invalidate would be the Loop session information. Shouldn't we also start a new Loop session? I'm trying to ensure that when the user logs out that nothing that they did in Loop while logged in will still be active as I would expect users to think of it like logging out of Skype: no more calls will be received.

It seems like a logout endpoint that ends the Loop session on the server (and thus deletes the push registration) would be worthwhile.

Adam, what do you think?

Comment 3

[Adam Roach [:abr]]

(In reply to Matthew N. [:MattN] from comment #2)
> Should we close the existing Web Socket Channel for Push after deleting the
> push registration? We would then get a new push URL and register it as a
> guest on the server. The remaining piece to invalidate would be the Loop
> session information. Shouldn't we also start a new Loop session? I'm trying
> to ensure that when the user logs out that nothing that they did in Loop
> while logged in will still be active as I would expect users to think of it
> like logging out of Skype: no more calls will be received.


This is close.

In practice, what we want to happen when a user logs out is that things should be in the state they were prior to the user logging in. This means that any non-account-associated "call-me" URLs should continue to work after user logout.

To make this work, I don't think we need to get a new push endpoint; all we need to do is do a "DELETE /registration" with the account, and then "POST /registration" without the account.

What is very important is that we don't invalidate or change the HAWK token, since that will prevent the non-account-associated URLs from working.

> It seems like a logout endpoint that ends the Loop session on the server
> (and thus deletes the push registration) would be worthwhile.

We may be talking past each other here. If I were to describe what "DELETE /registration" is, "a logout endpoint that ends the Loop session on the server (and thus deletes the push registration)" might well be how I do so.

Can you describe the difference between "DELETE /registration," as described in the link I gave in Comment 1, and what you think needs to happen here?

Comment 4

[Matthew N. [:MattN]]

(In reply to Adam Roach [:abr] from comment #3)
> (In reply to Matthew N. [:MattN] from comment #2)
> > Should we close the existing Web Socket Channel for Push after deleting the
> > push registration? We would then get a new push URL and register it as a
> > guest on the server. The remaining piece to invalidate would be the Loop
> > session information. Shouldn't we also start a new Loop session? I'm trying
> > to ensure that when the user logs out that nothing that they did in Loop
> > while logged in will still be active as I would expect users to think of it
> > like logging out of Skype: no more calls will be received.
> 
> This is close.
> 
> In practice, what we want to happen when a user logs out is that things
> should be in the state they were prior to the user logging in. This means
> that any non-account-associated "call-me" URLs should continue to work after
> user logout.
> 
> To make this work, I don't think we need to get a new push endpoint; all we
> need to do is do a "DELETE /registration" with the account, and then "POST
> /registration" without the account.

The username (email address) doesn't get associated to the account with POST /registration, it's with POST /fxa-oauth/token. It was decided that we should never pass the FxA OAuth token to the Loop server (which differs from what the docs for POST /registration say, likely because they are outdated). I guess I wouldn't expect that DELETE /registration would also disassociate any username with the user's existing session because I thought the endpoint was for "registration" of push URIs.

If you're saying that any usernames will be disassociated with the session when DELETE /registration is used then this can work but it seems unusual and possibly error prone for the server. I can see potential bugs where a push URI gets used for a user after they log out due to race conditions due to queues or caching. It seemed cleaner to start with a fresh session to avoid this class of issues although I didn't expect that we would want guest call links generated before the user logs in to continue to work after logging out (see below). 

It also seems unusual to me that we would DELETE and POST the same push URI to logout as I wouldn't guess that it meant I was logging out if I saw the requests go by. A …/logout endpoint which doesn't require passing a push URI seems more straightforward.

> What is very important is that we don't invalidate or change the HAWK token,
> since that will prevent the non-account-associated URLs from working.

I guess I'm not sure users will understand/remember the distinction between URLs generated while logged in and while logged out. Is this a documented requirement? I wonder what UX thinks about this. Darrin, do you have thoughts?

> Can you describe the difference between "DELETE /registration," as described
> in the link I gave in Comment 1, and what you think needs to happen here?

Answered above.

To be clear, I can see how it can work as you described but I think it could be clearer.

Comment 5

[Adam Roach [:abr]]

(In reply to Matthew N. [:MattN] from comment #4)
> The username (email address) doesn't get associated to the account with POST
> /registration, it's with POST /fxa-oauth/token. It was decided that we
> should never pass the FxA OAuth token to the Loop server (which differs from
> what the docs for POST /registration say, likely because they are outdated).
> I guess I wouldn't expect that DELETE /registration would also disassociate
> any username with the user's existing session because I thought the endpoint
> was for "registration" of push URIs.

Okay, I hadn't looked at Vlad's patches to this level of detail -- so it sounds like we need an equivalent DELETE endpoint that we can use to disassociate a specific user ID from a HAWK token. I'll dig into this a bit tomorrow and get a server bug opened on it, as it does appear that we need something new that the server isn't doing yet.

> > What is very important is that we don't invalidate or change the HAWK token,
> > since that will prevent the non-account-associated URLs from working.
> 
> I guess I'm not sure users will understand/remember the distinction between
> URLs generated while logged in and while logged out. Is this a documented
> requirement? I wonder what UX thinks about this. Darrin, do you have
> thoughts?

What we really want to avoid is a situation in which I've handed out several "call me" URLs, then later decide to create an account, log in, log out, and then have all my previously issued URLs stop working.

Comment 6

[Darrin Henein [:darrin]]

I think Adam clarified well here -- at the end of the day it shouldn't matter when/where call-me URLs were generated, they should continue to work until they expire or are revoked manually.

Comment 7

[Matthew N. [:MattN]]

Oh, so even call URLs generated while I'm logged-in should continue to work when I'm logged out? I was kinda thinking that wouldn't be the case as I could see a user thinking that they could logout to stop being bothered by a call URL from when they are logged in. Obviously there are better, more prominent ways the user can handle it (e.g. blocking the call, revoking the URL, or switching availability) that perhaps this isn't an issue so I'm fine with comment 6.

Comment 8

[Matthew N. [:MattN]]

Reminder about the server bug mentioned in comment 5.

Comment 9

[Adam Roach [:abr]]

(In reply to Matthew N. [:MattN] from comment #8)
> Reminder about the server bug mentioned in comment 5.

Okay, so I had a chat with Rémy about this over IRC, and it seems they aren't prepared to handle a HAWK token being used in an anonymous fashion *and* as an account-associated token. It's an either/or proposition.

So what we're going to need to do is keep track of two different HAWK tokens: one is associated with the browser, and stays the same across browser restarts. The other is associated with the user ID, and is generated when the user logs in.

Assuming a logged-in user: upon start-up, the loop client will need to get a single Push URL, and then register with the Loop server twice (using the same Push URL for both registrations): once with the browser HAWK token, and once with a user-associated HAWK token.

This means that the Loop client, when it gets a push notification, will need to check both sessions (the browser one and the user one) to see which calls are pending.

In this model, logging out involves deleting the HAWK token associated with the user, while leaving the one associated with the browser alone.

needinfo Maire: we probably need a little bit of additional bug breakdown for the FxA use cases based on this new understanding; basically: doing two Loop registrations, and checking two URLs for calls when we get an incoming call notification.

needinfo Rémy: please double-check my analysis above and make sure it matches your understanding of the server implementation.

Comment 10

[Rémy Hubscher (:natim)]

I agree with what you've described.

For now I imagine that if you are connected and create a call-url it will be set on your connected account rather than your anonymous one.

About the anonymous session, I think we should give the user a way to drop/logout from an anonymous session at some point.

If during my session on a public computer I am creating call-urls for a one or two calls, when I leave this computer I don't want people to be using my anonymous connection anymore (and then being able to get calls from my call-urls).

 - Can we logout from an anonymous session?
 - Should we just let the user revoke its URLs?
 - Should we ask the user when he close the browser and have created some anonymous call-urls: "Would you like to keep this call-urls or revoke them?"

I am not sure of the strategy we choose have but we should have one.

Can we start loop in privacy mode? If it is the case, maybe it is the solution for this case.
Then the anonymous session will stand only for the privacy mode session.

Comment 11

[Matthew N. [:MattN]]

Remy, can you confirmed that the server endpoints are already setup for the modified flow from comment 9. Can you spell out the endpoints used?

Comment 12

[Rémy Hubscher (:natim)]

Yes everything is ready on server 0.11 to handle what Adam described.

For BrowserID's and Anonymous login:

  - POST /registration with the simplePushURL you will get a Hawk-Session-Token header
  - Use this Hawk-Session-Token to use loop-server


For FxA-Oauth login:

  - POST /session 0.11 (that will probably be rename in /fxa-oauth/session for 0.12 according to bug 1059131)
  - Use the Hawk-Session-Token to do the fxa-oauth flow.
  - POST /registration with your simplePushURL to activate your push notification on this account.
  - Use your Hawk-Session-Token to use loop-server with your account.

Comment 13

[Matthew N. [:MattN]]

I have a patch for the registration changes in bug 1065052.

I believe for this bug that we need to do "DELETE /registration" while using the FxA Hawk Session. I'm hoping the server will do the right thing and not also delete the guest registration for the same simplePushURL.

Comment 14

[Matthew N. [:MattN]]

Rémy, is comment 13 correct about how to logout an FxA user?

Comment 15

[Rémy Hubscher (:natim)]

> We need to do "DELETE /registration" while using the FxA Hawk Session.

Yes the Hawk Session is the primary key so it will just remove the simplePushURL for the given user linked to that session.

Comment 16

[Matthew N. [:MattN]]

Great, thanks. I'll start on this tomorrow.

Comment 17

[Matthew N. [:MattN]]

Our Hawk/REST library doesn't support bodies on DELETE requests (and this seems to be affect some other libraries too) so we have to make a change somewhere:

> [16:49:26]  <ckarlof>	 1) we could make it a post
> [16:49:39]  <ckarlof>	 2) we could associate the push url with the session token in the server
> [16:49:47]  <ckarlof>	 3) we could make it a query param
> [16:49:59]  <ckarlof>	 4) we could change rest.js [to support bodies on DELETE] :)
> [16:50:25]  <ckarlof>	 1-3 requires a server change
> [16:50:30]  <ckarlof>	 4 sounds terrible

I would probably prefer #2, especially if the server is already associating only one pushURL to a hawk session.

Rémy/Alexis, what are your thoughts?

Comment 18

[Chris Karlof [:ckarlof]]

I looked at the server code, and it seems it is maintaining a list of push urls for each user and the list is associated with the user, not the token. This is why it need to the URL because it needs to know which URL to delete.

This could be re-factored, but it seems the simplest fix is to change the method to POST and the name to /delete-registration. This is likely a one line change to the code: 

https://github.com/ckarlof/loop-server/commit/d0df46be2ded54e82e6f575656f5dfc8963e57d8

Tests will need updating of course.

Comment 19

[Chris Karlof [:ckarlof]]

rest.js, which is used by our Hawk client doesn't support DELETE bodies:

http://mxr.mozilla.org/mozilla-central/source/services/common/rest.js#254

Comment 20

[Matthew N. [:MattN]]

Attachment: WIP 1. Patch without complete tests

Feel free to give feedback before I finish tests and wait for the server endpoint decision.

Comment 21

[Tarek Ziadé (:tarek)]

I think #2 makes a lot of sense

Comment 22

[Alexis Metaireau (:alexis)]

(Supposed to be on PTO until next monday, so answering here quickly)

If updating the APIs of the server can make things simpler to implement, then we should go for it, but I would of course prefer to have the client behave correctly.

Is there a technical limitation that prevents the rest.js file to handle DELETE bodies? Could we do that in parallel so that we deprecate the newly introduced API in the future in favor of the correct one?

One alternative proposal for the new APIs would be to have a DELETE on /registration/<simple-push-url> where the simple push url is url encoded. I prefer this because it keeps the DELETE verb and identifies clearly what's the resource that we're asking to delete.

Would that be simpler to implement on the client side?

Comment 23

[Alexis Metaireau (:alexis)]

Oh, and I realize I'm missing one big part of the discussion!

Yes, having the server using one simple push url per hawk session makes a lot of sense (the proposal number 2.) since the hawk sessions are meant to be unique for each user agent.

I believe that means some refactoring on the server, but I cannot tell how much without looking a bit at the code, and I would like to review what the needed changes are before committing to this.

Comment 24

[Tarek Ziadé (:tarek)]

(In reply to Alexis Metaireau (:alexis) from comment #22)
> I would of course prefer to have the client behave correctly.

Bodies on DELETE are quite exotic. (like on GETs)

> Is there a technical limitation that prevents the rest.js file to handle
> DELETE bodies? Could we do that in parallel so that we deprecate the newly
> introduced API in the future in favor of the correct one?

many libs out there don't implement bodies on idempotent methods. some proxies
are even dumb about this kind of things and throw errors. 

So if we can avoid it, it's less trouble

Comment 25

[Rémy Hubscher (:natim)]

> would be to have a DELETE on /registration/<simple-push-url>

I think I would prefer to keep the DELETE verb as well if possible.

Also, not to break other clients I would prefer to let the simple-push-url parameter be either in the body or in the querystring so that you can pass it in a querystring with your rest.js client.

We could add a key to track the simple push URL for a given hawk session but this may be another ticket.

Comment 26

[Rémy Hubscher (:natim)]

Attachment: Link to Github loop-server PR — #205

Here is my proposal to make it work for rest.js before changing the API later on if we want to make a 1:1 constrain between SessionToken and SimplePushURL

Comment 27

[Maire Reavy [:mreavy]]

(In reply to Rémy Hubscher (:natim) from comment #26)
> Created attachment 8488639 [details] [review]
> Link to Github loop-server PR — #205
> 
> Here is my proposal to make it work for rest.js before changing the API
> later on if we want to make a 1:1 constrain between SessionToken and
> SimplePushURL

If we could get this onto the dev server today, that'd be much appreciated. (To unblock MattN.)  Thanks!

Comment 28

[Matthew N. [:MattN]]

Option 5 would be to keep the same URL and method but put the simplePushURL in an HTTP header. The advantage of this over putting the URL in the path or query string is that it won't get logged by server logs (normally).

Comment 29

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #28)
> Option 5 would be to keep the same URL and method but put the simplePushURL
> in an HTTP header. The advantage of this over putting the URL in the path or
> query string is that it won't get logged by server logs (normally).

Looks like hawkclient.js doesn't really support that.

Comment 30

[Matthew N. [:MattN]]

Attachment: v.1 Patch with tests

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8489058 [details] [diff] [review]
v.1 Patch with tests

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

Looks good, will wait for updated patch before r+.

::: browser/components/loop/MozLoopAPI.jsm
@@ +384,5 @@
> +    logOutFromFxA: {
> +      enumerable: true,
> +      writable: true,
> +      value: function() {
> +        return MozLoopService.logOutFromFxA();

MozLoopService.logOutFromFxA only returns undefined.

::: browser/components/loop/MozLoopService.jsm
@@ +1058,5 @@
> +   * Logs the user out from FxA.
> +   *
> +   * Gracefully handles if the user is already logged out.
> +   */
> +  logOutFromFxA: Task.async(function*() {

At first I didn't see this Task.async call here, it kind-of blends in with the normal function declaration signature.

It may be easier for others to notice what is going on here if this is a regular function that returns the Task.async(function*() { ... }); body.

Otherwise, please add to the jsdoc comment above:
   * @return {Promise} that resolves when the FxA logout flow is complete.

::: browser/components/loop/content/js/panel.jsx
@@ -220,5 @@
>      },
>  
>      handleClickAuthEntry: function() {
>        if (this._isSignedIn()) {
> -        // XXX to be implemented - bug 979845

This change needs to be made to panel.js as well.

::: browser/components/loop/test/mochitest/loop_fxa.sjs
@@ +10,5 @@
>  const REQUIRED_PARAMS = ["client_id", "content_uri", "oauth_uri", "profile_uri", "state"];
>  const HAWK_TOKEN_LENGTH = 64;
>  
>  Components.utils.import("resource://gre/modules/NetUtil.jsm");
> +Components.utils.importGlobalProperties(["URL", "URLSearchParams"]);

URLSearchParams is unused.

Comment 32

[Matthew N. [:MattN]]

Attachment: v.2 Rebased patch on top of bug 1047146 addressing review comments

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #31)
> ::: browser/components/loop/MozLoopService.jsm
> @@ +1058,5 @@
> > +   * Logs the user out from FxA.
> > +   *
> > +   * Gracefully handles if the user is already logged out.
> > +   */
> > +  logOutFromFxA: Task.async(function*() {
> 
> At first I didn't see this Task.async call here, it kind-of blends in with
> the normal function declaration signature.
> 
> It may be easier for others to notice what is going on here if this is a
> regular function that returns the Task.async(function*() { ... }); body.

I see keeping the focus on the logic instead of the async boilerplate as an advantage of Tasks. This is also now a common pattern: https://mxr.mozilla.org/mozilla-central/search?string=%3A+Task.async
If async code has more overhead, people may continue to favour synchronous code.

> Otherwise, please add to the jsdoc comment above:
>    * @return {Promise} that resolves when the FxA logout flow is complete.

I will add the comment as it should have been there anyways. I prefer to leave the structure as-is to match other Task code in m-c I've worked with.

> ::: browser/components/loop/content/js/panel.jsx
> @@ -220,5 @@
> >      },
> >  
> >      handleClickAuthEntry: function() {
> >        if (this._isSignedIn()) {
> > -        // XXX to be implemented - bug 979845
> 
> This change needs to be made to panel.js as well.

Ah, I forgot this wasn't part of the build system and when I saw the .js didn't change I assumed it was because react-tools stripped comments.

> ::: browser/components/loop/test/mochitest/loop_fxa.sjs
> @@ +10,5 @@
> >  const REQUIRED_PARAMS = ["client_id", "content_uri", "oauth_uri", "profile_uri", "state"];
> >  const HAWK_TOKEN_LENGTH = 64;
> >  
> >  Components.utils.import("resource://gre/modules/NetUtil.jsm");
> > +Components.utils.importGlobalProperties(["URL", "URLSearchParams"]);
> 
> URLSearchParams is unused.

Yep, leftover from an earlier version.

Comment 33

[Alexis Metaireau (:alexis)]

It seems that most of the clients out there (not only gecko) aren't currently supporting DELETE with a body, so that's why we shouldn't go with it. 

I was wrong with my assumption that the client was behaving in an abnormal way.

Comment 34

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/43b24197d25a

Comment 35

[Matthew N. [:MattN]]

Thanks

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/a660e08ac3b2 to be able to cleanly backout bug 1047181. Feel free to reland this whenever you get to it.

Comment 37

[James Bonacci [:jbonacci]]

This appears to be a blocker on the server and client sides, if I am understanding things.
I will temporarily hold up the latest Loop-Server deploy (0.12.2) until I hear from all of you and from the EU team on just what needs to be Fixed and released.

Comment 38

[James Bonacci [:jbonacci]]

OK, so this is now holding up the current Loop-Server deploy which is already late.
:tarek
:natim 
Can you yank out all the server-specific changes from this bug and open a new Loop-Server bug?

Comment 39

[Alexis Metaireau (:alexis)]

James, the server bits are already done and are part of the 0.12.0 release (see https://github.com/mozilla-services/loop-server/blob/master/CHANGELOG#L29-L30), so this should not block the current loop-server deploy.

Comment 40

[James Bonacci [:jbonacci]]

Removing blocker tag.

We will move forward with the Loop-Server side.
If there is a Loop-Client deploy required, please let us know ASAP.

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

Relanded with a test fix in bug 1047181,
https://hg.mozilla.org/integration/fx-team/rev/0519bf931920

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0519bf931920

Comment 43

[u279076]

Paul, please test this to verify it's working in the latest Nightly and the following Try-server build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f9eb2cbac352

Comment 44

[Paul Silaghi, QA [:pauly]]

So, I see the call URLs generated while I'm logged out continue to work when I'm logged in.
But the call URLs generated while I'm logged in no longer work when I'm logged out - the caller is "connecting" for 10 seconds, then the call ends.
Is this expected ?
Are there other scenarios to test here ?

Also, if I logout from the account page, but still logged in the Loop panel (bug 1072965), the call URLs generated while I was logged in still work.

Comment 45

[Matthew N. [:MattN]]

(In reply to Paul Silaghi, QA [:pauly] from comment #44)
> So, I see the call URLs generated while I'm logged out continue to work when
> I'm logged in.

That's good.

> But the call URLs generated while I'm logged in no longer work when I'm
> logged out - the caller is "connecting" for 10 seconds, then the call ends.
> Is this expected ?

Yes, see bug 1065153 comment 23 part A.

> Are there other scenarios to test here ?

You can confirm that the pref loop.hawk-session-token.fxa is cleared upon logout.

> Also, if I logout from the account page, but still logged in the Loop panel
> (bug 1072965), the call URLs generated while I was logged in still work.

Being logged into the account page just means you are logged into Firefox Accounts and not necessarily Loop. The panel footer (and pref loop.hawk-session-token.fxa) is what matters so don't look at the account page to know if you're logged into Loop.

Comment 46

[u279076]

(In reply to Matthew N. [:MattN] from comment #45)
> You can confirm that the pref loop.hawk-session-token.fxa is cleared upon
> logout.

Paul, if you can confirm this we should be all good to mark this verified.

Comment 47

[Paul Silaghi, QA [:pauly]]

Verified fixed 35.0a1 (2014-10-01) and http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f9eb2cbac352

Comment 48

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8491132 [details] [diff] [review]
v.2 Rebased patch on top of bug 1047146 addressing review comments

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 49

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/31c37e8b51dd
https://hg.mozilla.org/releases/mozilla-aurora/rev/0e6824c0d717

Comment 50

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8491132 [details] [diff] [review]
v.2 Rebased patch on top of bug 1047146 addressing review comments

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.

Comment 51

[Alexis Metaireau (:alexis)]

(In reply to Adam Roach [:abr] from comment #9)
> In this model, logging out involves deleting the HAWK token associated with
> the user, while leaving the one associated with the browser alone.

Sorry, I'm really late to the party.

I don't understand why we would need to delete a hawk token associated to a user when he logs out? What logging out means, from my perspective, is that the user is not reachable anymore. E.g. her simple push url is not in the server database so we cannot ping her.

However, logging out shouldn't mean deleting the hawk session on the server. Adam, why would we want that? (am I missing something?)

Comment 52

[Adam Roach [:abr]]

(In reply to Alexis Metaireau (:alexis) from comment #51)
> However, logging out shouldn't mean deleting the hawk session on the server.
> Adam, why would we want that? (am I missing something?)

To prevent unbounded state growth? I guess the question I would have is why we'd want to keep it around. We just get a new one when we log in, right?

Comment 53

[Alexis Metaireau (:alexis)]

These hawk sessions expires automatically after some time (defined at the discretion of the server, currently one month [0]).

Keeping them makes sense so we do not need to do the OAuth flow again or the BrowserId verification again. And if it's expired, then you do the roundtrip again.

[0] https://github.com/mozilla-services/loop-server/blob/master/loop/config.js#L277-L281

Comment 54

[Matthew N. [:MattN]]

If the user clicks logout then we should be deleting the session on the server as a normal security/privacy practice.

Consider the case where a user is on a shared computer:
If someone had captured the session token e.g. via spyware/malware it should be of no use after the logout happens since the server should have deleted the session. This is how every website with logins sessions that I know of works.

Note that we don't unregister upon shutdown/restart. This bug is only about when the user explicitly choose to log out from the gear menu.

Comment 55

[Alexis Metaireau (:alexis)]

Then in this case, it seems to make sense to me to have the clients call DELETE /session rather than DELETE /registration.

Would that work for you?

Comment 56

[Adam Roach [:abr]]

(In reply to Alexis Metaireau (:alexis) from comment #55)
> Then in this case, it seems to make sense to me to have the clients call
> DELETE /session rather than DELETE /registration.
> 
> Would that work for you?

That seems like an unnecessarily disruptive change for a gain that is very difficult to quantify.

Comment 57

[Alexis Metaireau (:alexis)]

It's just that the API wouldn't be consistent. a DELETE on /registration should remove the registration. If we want to remove the session then we should do a DELETE /session.

Comment 58

[Adam Roach [:abr]]

(In reply to Alexis Metaireau (:alexis) from comment #57)
> It's just that the API wouldn't be consistent. a DELETE on /registration
> should remove the registration. If we want to remove the session then we
> should do a DELETE /session.

I don't want to sound unsympathetic to making things cleaner, but you realize that this code is in Beta now, right? It would be hard to justify uplifting this change to Beta when there is no user-visible broken behavior.

Comment 59

[Alexis Metaireau (:alexis)]

I think uplifting this change would make sense, even if there is no user-visible behavior. It's a mistake on the client and we shouldn't change the server semantics just to allow mistakes to continue.

I believe the right way to do things would be to fix the client and make it ride the trains + uplift.

In any case, the client needs to be updated to act the "right" way.