Closed Bug 1153788 Opened 9 years ago Closed 9 years ago

Obtain encryption keys from FxA for Loop

Categories

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

defect
Points:
5

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Iteration:
40.3 - 11 May
Tracking Status
firefox40 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Whiteboard: [context][ux])

Attachments

(3 files, 4 obsolete files)

Bug 1142522 implemented context in conversations for guests (non-signed in). We need to extend this to FxA so that signed-in users can use encrypted context as well.
Rank: 5
No longer depends on: 1142522
Depends on: 1142522
Flags: firefox-backlog+
Assignee: nobody → standard8
Iteration: --- → 40.2 - 27 Apr
Depends on: 1132293
Sevaan, due to the way the FxA encryption keys work, we can only get an encryption key from FxA if the user signs-in. So for existing users on upgrade, we won't be able to get a key.

If we can't get a key, we won't be able to store any context around the conversation. Whilst we could generate a temporary key, it wouldn't be available on any other devices that the user signed into, and we'd need to re-encrypt everything when they re-signed in, and there's not guarantees this would happen.

So I think we need to get them to sign back in again somehow, and hence I'm looking for some UX thoughts please!
Flags: needinfo?(sfranks)
rfeeley, what do you suggest happen here? Is there precedent for this anywhere else in the FxA world?
Flags: needinfo?(sfranks) → needinfo?(rfeeley)
Whiteboard: [context]
This is an intermediate step whilst we're waiting for UX - If the user logs in, then we save the key, and remember it (which we'll be doing anyway). We won't do the sign-out/in dance until we've got UX for it.
Attachment #8595483 - Flags: review?(mdeboer)
Comment on attachment 8595483 [details] [diff] [review]
Part 1 Opportunistically obtain encryption keys for FxA for Loop conversations context.

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

I think this is good to go for now. I hope we can get some clarity around the re-signin flow soon.

::: browser/components/loop/MozLoopService.jsm
@@ +52,5 @@
>  Cu.import("resource://gre/modules/Task.jsm");
>  Cu.import("resource://gre/modules/Timer.jsm");
>  Cu.import("resource://gre/modules/FxAccountsOAuthClient.jsm");
>  
> +Cu.importGlobalProperties(["URL", "crypto"]);

I don't think this added property is used anywhere.

::: browser/components/loop/content/js/roomViews.js
@@ +391,5 @@
>      },
>  
>      _shouldRenderContextView: function() {
>        return !!(
> +        this.props.mozLoop.getLoopPref("contextInConverations.enabled") &&

Please revert this change.

::: browser/components/loop/content/js/roomViews.jsx
@@ +391,5 @@
>      },
>  
>      _shouldRenderContextView: function() {
>        return !!(
> +        this.props.mozLoop.getLoopPref("contextInConverations.enabled") &&

please revert this change.
Attachment #8595483 - Flags: review?(mdeboer) → review+
I'll defer to Ryan Kelly, our engineering lead on Firefox Accounts.
Flags: needinfo?(rfeeley) → needinfo?(rfkelly)
> Is there precedent for this anywhere else in the FxA world?

The closest thing we have to precedent is sync.  If sync finds itself in a state where it doesn't have the right encryption keys, it will stop working and display a message asking the user to re-authenticate.  Hopefully we can do better here...

A related case is if the user resets their password.  This will cause the master encryption keys on the account to change.  So in addition to "we don't have the key yet" you also need to deal with "we have an outdated key".

Can we put the Hello client code into some sort of "pending re-authentication" state where the core functionality will continue to work, but we opportunistically prompt you for login on certain actions?

rfeeley, perhaps we can treat this analogously to your proposed sync-error-bar changes, but applied to the Hello UI elements?  This one:

  https://www.dropbox.com/s/4exxkl19cg0gowg/sync-reconnect1-rough.png?dl=0
Flags: needinfo?(rfkelly) → needinfo?(rfeeley)
Most of the states that need to accommodated on the client side are accounted for in our guidelines under "Additional States" https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Firefox_Accounts/UX_guidelines
Flags: needinfo?(rfeeley)
Whiteboard: [context] → [context][ux]
Mark, do we have this error bar implemented? https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#error

If so, we could put a message in there asking users to re-sign in.
Iteration: 40.2 - 27 Apr → 40.3 - 11 May
(In reply to Sevaan Franks [:sevaan] from comment #10)
> Mark, do we have this error bar implemented?
> https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#error
> 
> If so, we could put a message in there asking users to re-sign in.

Yes we do.

Do we also want to disable the start conversation / edit context options until they've re-signed in? Those options wouldn't work if we didn't have a key.
Flags: needinfo?(sfranks)
There are a couple questions before we can answer that...

(In reply to Ryan Kelly [:rfkelly] from comment #7)
> Can we put the Hello client code into some sort of "pending
> re-authentication" state where the core functionality will continue to work,
> but we opportunistically prompt you for login on certain actions?

That's a good idea. Could we have non-context conversations work fine, but if context is added, or context in an existing conversation is being edited, we prompt the login more explicitly?

Out of curiosity, if a user is already logged in, couldn't we just re-resign them in behind the scenes and get the encryption keys that way?
Flags: needinfo?(sfranks)
> Out of curiosity, if a user is already logged in, couldn't we just re-resign
> them in behind the scenes and get the encryption keys that way?

I don't think so, sadly.  The great joy of using password-derived encryption keys is that you must prompt for the password whenever you need to re-acquire the keys.
Depends on: 1161560
As I now understand it, the Conversation Title is now part of the room context, so it requires encryption keys as well.

If this is the case, then I don't think that error message UI at the top of the Hello panel will suffice as no functionality will work until the user re-signs in. It would be better just to put an overlay over the whole panel with messaging and the FxA login fields, forcing the user through a funnel to re-sign in before they can do anything else in Hello. We can also give the user the option to disconnect the account, reverting them back to Guest mode.
Attached image Re-sign in Panel for Firefox Hello (obsolete) —
If a user is signed in to Firefox Hello, the attached panel should display the first time they open the Hello panel upon updating the browser.

"Forgot Password" and "Use a Different Account" opens those appropriate Account pages in a new tab and closes the panel

"Use Hello as a Guest" signs the user out, removes the sign-in panel, and shows the standard non-signed in Hello interface.

If a user logs in, the submit button loading icon used on the account pages is displayed, and upon logging in the sign-in panel is removed and the signed-in Hello interface is displayed.

Room notifications can still occur, and the user may click on them to open the Conversation Window. However, the edit context button will be disabled until the user has signed in through the panel.
Attachment #8602388 - Flags: ui-review?(rfeeley)
I chatted with some of the FxA folks today about the possibilities for the UX in comment 15.

Practically, we'd need to embed the web pages in an iframe or something in the panel, and do the log-in from there. We'd need to be able to handle things like the user being offline, and ensure that we managed the security correctly, since there wouldn't be a url bar. This could be a reasonable amount of work, but I'll take an experimental look at it.

A simpler, but not so ideal UX, would be to have a button to open a tab like we do currently.
(In reply to Mark Banner (:standard8) from comment #16)
> A simpler, but not so ideal UX, would be to have a button to open a tab like
> we do currently.

+1 as this would be significantly less work and more realistic for the short term. As for the longer term, the proposed UX looks a-ok.
Depends on: 1162495
Attached image Simple Panel (obsolete) —
I'm all for going the easier route in this case. Button only version attached.
Attachment #8602724 - Flags: ui-review?(rfeeley)
This implements the simple button UX that Sevaan posted. Note that it depends on bug 1162495 as we need that functionality as well.
Attachment #8595483 - Attachment is obsolete: true
Attachment #8602785 - Flags: review?(mdeboer)
Blocks: 1162570
Hi Bogdan,

There are a group of bugs landing for the new feature Hello is adding called Context.  The Master bug that has user stories under it is Bug 1115340, all related bugs are tagged in the whiteboard as [context].  After this current bug, bug 1153788 lands it should be ready for testing the feature functionality.  

The only bit that won't be done (as it's lower priority/risk) is the flavicon finished implementation and some polish).

The main UX for the bug is under bug 1153788, except for the portion on the re-authentication - which is here.
Flags: needinfo?(bogdan.maris)
Comment on attachment 8602785 [details] [diff] [review]
Part 2. Ask the user to re-sign in to Loop if they don't have encryption keys for FxA.

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

This looks good to me, except for how the styling is applied. I'd appreciate one additional iteration on that.

::: browser/components/loop/content/css/panel.css
@@ +31,5 @@
> +  margin: 2em 0;
> +}
> +
> +.sign-in-request > h1 {
> +  font-size: 1.70em;

nit: please drop the `0`.

@@ +37,5 @@
> +}
> +
> +.sign-in-request > h2,
> +.sign-in-request > a {
> +  font-size: 1.20em;

nit: please drop the `0`.

@@ +40,5 @@
> +.sign-in-request > a {
> +  font-size: 1.20em;
> +}
> +
> +.sign-in-request > a {

Can you underline the link on hover and active states?

@@ +48,5 @@
> +
> +.sign-in-request-button {
> +  font-size: 1rem;
> +  margin: 1rem;
> +  width: 80%;

I'm not really a fan if dynamic width settings like this. I'm sure this looks OK, but I'd rather you changed this layout to use the flex-box model, which stretches elements dynamically, whilst taking exact margins specified on the element into account.

Would you like to try that?

::: browser/components/loop/content/js/panel.jsx
@@ +234,5 @@
> +      this.props.mozLoop.logOutFromFxA();
> +    },
> +
> +    render: function() {
> +      var signInText = "Sign In";

Please remove this left-over.

@@ +988,5 @@
>      React.render(<PanelView
>        notifications={notifications}
>        roomStore={roomStore}
>        mozLoop={navigator.mozLoop}
> +      dispatcher={dispatcher} />, document.querySelector("#main"));

Hehe, I see your editor doesn't like this notation either! :-P

::: browser/components/loop/modules/MozLoopService.jsm
@@ +964,5 @@
>          parameters.keys = true;
> +        if (forceReAuth) {
> +          parameters.action = "force_auth";
> +          parameters.email = MozLoopService.userProfile.email;
> +          MozLoopService.log.error(parameters);

Why error?
Attachment #8602785 - Flags: review?(mdeboer) → review-
(In reply to Mike de Boer [:mikedeboer] from comment #21)
> @@ +48,5 @@
> > +
> > +.sign-in-request-button {
> > +  font-size: 1rem;
> > +  margin: 1rem;
> > +  width: 80%;
> 
> I'm not really a fan if dynamic width settings like this. I'm sure this
> looks OK, but I'd rather you changed this layout to use the flex-box model,
> which stretches elements dynamically, whilst taking exact margins specified
> on the element into account.
> 
> Would you like to try that?

I've given it a try, but I don't really see that having flex box column is going to help the horizontal width. It might help some of the vertical items, but it doesn't feel worth it at the moment as the panel is largely static in size.

> ::: browser/components/loop/modules/MozLoopService.jsm
> @@ +964,5 @@
> >          parameters.keys = true;
> > +        if (forceReAuth) {
> > +          parameters.action = "force_auth";
> > +          parameters.email = MozLoopService.userProfile.email;
> > +          MozLoopService.log.error(parameters);
> 
> Why error?

That was debug mistakenly left in ;-)
Updated patch for review comments.
Attachment #8602785 - Attachment is obsolete: true
Attachment #8603295 - Flags: review?(mdeboer)
Comment on attachment 8603295 [details] [diff] [review]
Part 2. Ask the user to re-sign in to Loop if they don't have encryption keys for FxA.

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

Nice! LGTM.

::: browser/components/loop/content/css/panel.css
@@ +46,5 @@
> +  color: #0295df;
> +}
> +
> +.sign-in-request > a:hover,
> +.sign-in-request > a:active {

`a:hover:active {`

::: browser/components/loop/content/js/panel.jsx
@@ +912,5 @@
>          );
>        }
>  
> +      if (!this.state.hasEncryptionKey) {
> +        return <SignInRequestView mozLoop={this.props.mozLoop}/>;

nit: I think we like to see a space before `/>`, right?
Attachment #8603295 - Flags: review?(mdeboer) → review+
(In reply to Mark Banner (:standard8) from comment #22)
> I've given it a try, but I don't really see that having flex box column is
> going to help the horizontal width. It might help some of the vertical
> items, but it doesn't feel worth it at the moment as the panel is largely
> static in size.

Gotcha. Well, since it's only an implementation detail I don't think it matter _that_ much.
Status: NEW → ASSIGNED
   12.12 +## LOCALIZATION_NOTE(sign_in_again_title_line_one, sign_in_again_title_line_two):
   12.13 +## These are displayed together at the top of the panel when a user is needed to
   12.14 +## sign-in again. The first "line_one" is slightly bigger font that "line_two",
   12.15 +## hence the separation.
   12.16 +sign_in_again_title_line_one=Please sign in again
   12.17 +sign_in_again_title_line_two=to continue using Firefox Hello

This is terrible i18n, because you're making a lot of assumptions: 
* That localizations will be able to keep the "sign in" in the first part (I assume the first part is bigger because you want to emphasize the need to sign-in again).
* That the first string will be shorter than the second.

Besides that, any reason not to use clientShortname2 and clientSuperShortname instead of hard-coded brand names?
(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #28)
> This is terrible i18n, because you're making a lot of assumptions: 
> * That localizations will be able to keep the "sign in" in the first part (I
> assume the first part is bigger because you want to emphasize the need to
> sign-in again).
> * That the first string will be shorter than the second.
> 
> Besides that, any reason not to use clientShortname2 and
> clientSuperShortname instead of hard-coded brand names?

Good points, about the clientShortname2 and clientSuperShortname for sure. Mark, can you take a look?
Flags: needinfo?(standard8)
The split-string UX is based on the existing Firefox Accounts sign-in display (I think there's a few other variants they have already). I've changed the strings here to try and improve options for localisers a bit, however if that's not sufficient, please file a new bug and it can be dealt with there.

This also puts in the brand name replacements.
Attachment #8604038 - Flags: review?(mdeboer)
Comment on attachment 8604038 [details] [diff] [review]
Follow-up to bug 1153788. Improve strings for L10n - use existing replacements for brandname and improve the localisation notes.

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

Nice, thanks for the quick follow-up!
Attachment #8604038 - Flags: review?(mdeboer) → review+
Flags: needinfo?(standard8)
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(In reply to :shell escalante from comment #20)
> Hi Bogdan,
> 
> There are a group of bugs landing for the new feature Hello is adding called
> Context.  The Master bug that has user stories under it is Bug 1115340, all
> related bugs are tagged in the whiteboard as [context].  After this current
> bug, bug 1153788 lands it should be ready for testing the feature
> functionality.  
> 
> The only bit that won't be done (as it's lower priority/risk) is the
> flavicon finished implementation and some polish).
> 
> The main UX for the bug is under bug 1153788, except for the portion on the
> re-authentication - which is here.

I`m on top of this for some time now, will keep verifying bugs and helping on this as much as time allows, just kept the needinfo as a reminder :)
Flags: needinfo?(bogdan.maris)
Discussed with Sevaan. I recommend matching the visual style to the Pocket style, but otherwise the actions Sevaan is proposing are correct.
(In reply to Ryan Feeley from comment #34)
> Discussed with Sevaan. I recommend matching the visual style to the Pocket
> style, but otherwise the actions Sevaan is proposing are correct.

CORRECTION: Hello style, not Pocket.
Attached image Sign In Again Panel
Updated mockup. Changed button to blue.
Attachment #8602388 - Attachment is obsolete: true
Attachment #8602724 - Attachment is obsolete: true
Attachment #8602388 - Flags: ui-review?(rfeeley)
Attachment #8602724 - Flags: ui-review?(rfeeley)
One small change...a small g in "Use Hello as a guest"
You need to log in before you can comment on or make changes to this bug.