1039987 - Notify the user when resetting the hawk session token for non-accounted clients

Status: RESOLVED WORKSFORME

Description

[Mark Banner (:standard8)]

In bug 1038699 & others, we reset the hawk session token if there's a problem with the token, and we can't recover. This will mean that the user's previously sent urls will be invalid.

Whilst this generally shouldn't happen we should still have a notification to the user if it does happen.

From bug 1038699, the recommended UX is:

- Display a warning icon in the toolbar
- In the panel, display a warning.

Comment 1

[Rémy Hubscher (:natim)]

Also if the SimplePush URL is still the same, we could get notified of a call without seeing it to the GET /calls endpoint.

I suppose in case of new hawkToken we should register a new SimplePush URL.

Comment 2

[Maire Reavy [:mreavy] Plz needinfo]

This feels like an edge case, so I'd like to move it to fx35, but I want to talk to Mark first.

Comment 3

[Mark Banner (:standard8)]

Hmm, this is pretty bad if we don't it and manage to hit it - the user would have no idea that their previously sent urls are no longer valid.

Comment 4

[Maire Reavy [:mreavy] Plz needinfo]

Thanks for the info, Mark.  I think we could ship without this, but it would good to get this into Fx34.  So I'm changing the priority to P2.

Comment 5

[Maire Reavy [:mreavy] Plz needinfo]

I'd like to get this into Fx 34 if possible.  

Darrin -- Would we require new strings for this or can we reuse strings from other mock ups?

Comment 6

[Darrin Henein [:darrin]]

I don't think I had any real errors in the spec/mockup, as none were enumerated in the original stories.

I'm thinking we'll need something like "Session Expired - Please login again."... does this make sense as a user-facing error for this issue?

Comment 7

[:shell escalante]

HI Mark - does the wording Darrin put into comment 6 work for this bug?  if yes then we have the missing info for when this bug is ready.

Comment 8

[Mark Banner (:standard8)]

Not quite, the text is assuming we're logged in, but this is for the anonymous case.

I would suggest something like "Session Expired, all previously generated urls are no longer valid".

Comment 9

[Mark Banner (:standard8)]

(needinfo to Darrin to see what he thinks of the suggested text).

Comment 10

[Darrin Henein [:darrin]]

Please needinfo Arcadio or Matej for UI copy related questions.

Comment 11

[Matej Novak [:matej]]

(In reply to Mark Banner (:standard8) from comment #8)
> Not quite, the text is assuming we're logged in, but this is for the
> anonymous case.
> 
> I would suggest something like "Session Expired, all previously generated
> urls are no longer valid".

Maybe this could use a little more explanation:

Session expired. All URLs you have previously created and shared are no longer valid.

OR:

Session expired. All URLs you have previously created and shared will no longer work.

WDYT?

Comment 12

[Mark Banner (:standard8)]

(In reply to Matej Novak [:matej] from comment #11)
> Session expired. All URLs you have previously created and shared will no
> longer work.

This sounds reasonable, lets go with this one.

Comment 13

[Mark Banner (:standard8)]

Waiting on bug 1002416 to do this, as that has restructured the error handling and notifications.

Comment 14

[Mark Banner (:standard8)]

Attachment: [checked in] Add string for notifying the user that the session had expired.

This adds the required string for now. We should be able to land the full patch in the next day or so, once bug 1002416 lands.

Comment 15

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8482316 [details] [diff] [review]
[checked in] Add string for notifying the user that the session had expired.

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

LGTM.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8482316 [details] [diff] [review]
[checked in] Add string for notifying the user that the session had expired.

https://hg.mozilla.org/integration/fx-team/rev/1230a7a5c04b

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1230a7a5c04b

Comment 18

[:shell escalante]

this is marked leave open - are we still going to try and check in?  the string was added - so what else remains for the work?  is this fx35 or fx36

Comment 19

[Mark Banner (:standard8)]

(In reply to sescalante from comment #18)
> this is marked leave open - are we still going to try and check in?  the
> string was added - so what else remains for the work?  is this fx35 or fx36

Well, it would be to do the work, but I think MattN did this in bug 1047164.

Matt, do you think there's anything else to do here? Was there a follow-up somewhere for making it stay on the screen for a longer time?

Comment 20

[Matthew N. [:MattN] (PM me if requests are blocking you)]

(In reply to Mark Banner (:standard8) from comment #19)
> (In reply to sescalante from comment #18)
> > this is marked leave open - are we still going to try and check in?  the
> > string was added - so what else remains for the work?  is this fx35 or fx36
> 
> Well, it would be to do the work, but I think MattN did this in bug 1047164.
> 
> Matt, do you think there's anything else to do here? Was there a follow-up
> somewhere for making it stay on the screen for a longer time?

Bug 1076652 was filed for having it stay around.

The following wasn't implemented  (only showing of the warning bar was) so this bug could morph to cover that:

(In reply to Rémy Hubscher (:natim) from comment #1)
> Also if the SimplePush URL is still the same, we could get notified of a
> call without seeing it to the GET /calls endpoint.
> 
> I suppose in case of new hawkToken we should register a new SimplePush URL.

Comment 21

[:shell escalante]

Mark, as we comfortable putting until 37 rather than uplifting for Fx35 based on infrequency.  Please let me know if we caused heartburn putting off uplifting this last piece Matt mentioned.

Comment 22

[Mark Banner (:standard8)]

With the server guys talking about database migrations and potentially not migrating data etc, I'm not keen on delaying this, but given that they seem to be trying to preserve data, then hopefully its ok.

Comment 23

[Adam Roach [:abr]]

#include <std/rant_about_loop_is_not_anonymous>

Comment 24

[Romain Testard [:RT]]

I'm a bit confused about the status on this one are we waiting on bug 1076652 to be implemented?

Comment 25

[Mark Banner (:standard8)]

(In reply to Romain Testard [:RT] from comment #24)
> I'm a bit confused about the status on this one are we waiting on bug
> 1076652 to be implemented?

Re-reading this, I think we just need bug 1076652 implementing, so I'll close this as WFM.

The comment from Remy in comment 20 I don't think applies - if we register and find the token is invalid, then the registration won't have happened, so we'll clear the token and re-register with the same push url. However, as that hasn't registered, that should be fine (and even if it isn't we are able to cope with spurious push notifications).

Comment 26

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.