feat(oauth): show permission screen for untrusted reliers #2377

zaach · 2015-05-05T02:25:56Z

I refactored sign up/in to use the account and user models, but still need to get the permission screen working.

shane-tomlinson · 2015-05-05T19:04:50Z

app/scripts/templates/permissions.mustache

@@ -0,0 +1,30 @@
+<div id="main-content" class="card">
+  <header>
+    <h1 id="fxa-confirm-header">{{#t}}%serviceName)s would like to know...{{/t}}</h1>


%(serviceName)s

vladikoff · 2015-05-05T19:11:22Z

app/scripts/views/permissions.js

+],
+function (Cocktail, _, FormView, BaseView, Template, p, AuthErrors, Constants,
+    URL, ResendMixin, BackMixin, ServiceMixin) {
+  //var t = BaseView.t;


extra comment

pdehaan · 2015-05-05T19:16:52Z

app/scripts/views/permissions.js

+        email: account.get('email'),
+        serviceName: this.relier.get('serviceName'),
+        termsUri: this.relier.get('termsUri'),
+        privacyUri: this.relier.get('termsUri'),


this.relier.get('privacyUri')?

shane-tomlinson · 2015-05-06T19:37:48Z

app/scripts/models/reliers/oauth.js

+      return this.get('trusted');
+    },
+
+    needsPermissions: function (account) {


I'm waffling whether this should be on the relier on on the account. I can see it both ways.

Ask the account if the relier needs permission to access its information.
vs
Ask the relier if it needs permission to access the account.

The first makes more sense to me.

I was also considering the broker? Dunno.

shane-tomlinson · 2015-05-06T19:38:10Z

app/scripts/models/account.js

@@ -174,6 +175,54 @@ define([
        .then(function () {
          return profileImage;
        });
+    },
+
+    signIn: function (relier) {


I totally dig this.

shane-tomlinson · 2015-05-06T19:39:32Z

app/scripts/models/user.js

@@ -222,6 +222,22 @@ define([
      return (Session.email && Session.sessionToken &&
        (! Session.cachedCredentials ||
        Session.cachedCredentials.email !== Session.email));
+    },
+
+    signInAccount: function (account, relier) {


YES! I'll be happy to update the PR I showed you to continue with these changes and rip a lot of this account management logic out of the views.

shane-tomlinson · 2015-05-06T19:41:05Z

app/scripts/templates/permissions.mustache

@@ -0,0 +1,30 @@
+<div id="main-content" class="card">
+  <header>
+    <h1 id="fxa-confirm-header">{{#t}}%(serviceName)s would like to know...{{/t}}</h1>


Do you want the id to be fxa-permissions-header or something different than confirm?

shane-tomlinson · 2015-05-06T19:43:45Z

app/scripts/templates/permissions.mustache

+    </div>
+
+    <div class="button-row">
+      <a id="proceed" class="button" href="#">{{#t}}Proceed{{/t}}</a>


Not a normal submit button like the other templates?

shane-tomlinson · 2015-05-06T19:46:06Z

app/scripts/views/permissions.js

+
+    context: function () {
+      var account = this.getAccount();
+      var serviceUri = URL.getOrigin(this.relier.get('redirectUri'))


Something we may want to put on the relier when the redirectUri is set?

shane-tomlinson · 2015-05-06T19:48:21Z

app/scripts/views/permissions.js

+
+    onSignUpSuccess: function (account) {
+      var self = this;
+      if (account.get('verified')) {


We so need an external state machine and to rip this stuff out of the views.

shane-tomlinson · 2015-05-06T19:49:03Z

app/scripts/models/account.js

+        });
+    },
+
+    saveGrantedPermissions: function (clientId, scope) {


I do not see this called anywhere.

Yeah, still WIP

shane-tomlinson · 2015-05-06T19:50:24Z

app/scripts/views/sign_in.js

-        return self.user.setSignedInAccount(account)
-          .then(function () {
-            return account;
+        console.log('account???', account.get('sessionToken'), account);


shane-tomlinson · 2015-05-06T19:50:39Z

app/scripts/views/sign_in.js

-          return p.reject(AuthErrors.toError('UNEXPECTED_ERROR'));
-        }
+      return self.broker.beforeSignIn(account.get('email'))
+      .then(function () {


Looks like an opportune time to re-indent this.

shane-tomlinson · 2015-05-06T19:58:39Z

Overall, I dig this approach.

I would really like to see us move towards ripping the implicit state machine out of the views and into something self contained and duplicate logic can be reduced.

shane-tomlinson · 2015-05-06T20:01:07Z

app/scripts/models/reliers/oauth.js

+    },
+
+    needsPermissions: function (account) {
+      return ! this.isTrusted() && ! account.hasGrantedPermissions(this.get('clientId'), this.get('scope'));


Minor nit, but the intention could probably be easier to understand if it were re-written a bit like:

if (this.isTrusted()) { return false; } return ! account.hasGrantedPermissions(...

My guess is uglify will combine the two branches into something similar to what you have here.

shane-tomlinson · 2015-05-06T20:04:16Z

app/scripts/views/sign_in.js

-            return account;
+        console.log('account???', account.get('sessionToken'), account);
+        if (self.relier.needsPermissions(account)) {
+          self.navigate('signin_permissions', {


How you pass the account is slightly different to how its done in onSignInUnverified

shane-tomlinson · 2015-05-06T20:07:12Z

app/scripts/views/sign_up.js

      var preVerifyToken = self.relier.get('preVerifyToken');
+      var account = self.user.initAccount({
+        email: self.$('.email').val(),


At least the email should be updated to use getElementValue so that leading/trailing whitespace is trimmed.

Side note - how have we gotten away with using .val() for so long?

shane-tomlinson · 2015-05-06T20:11:31Z

app/scripts/views/permissions.js

+      }
+    },
+
+    onSignUpSuccess: function (account) {


Perhaps to avoid duplication with the signin/signup modules, these three functions could be extracted into one ore two mixins (one for sign in, one for sign up? overkill?) that are mixed in where needed?

shane-tomlinson · 2015-05-06T20:13:25Z

app/scripts/views/permissions.js

+  'views/form',
+  'views/base',
+  'stache!templates/permissions',
+  'lib/promise',


I don't think p is used anywhere in this module.

Ooohhh!!! I think i saw a grunt task for this the other day: grunt-amdcheck.

I did a quick test against master with this, grunttasks/amdcheck.js:

module.exports = function (grunt) { grunt.config('amdcheck', { app: { files: [{ expand: true, cwd: 'app/', src: [ '**/*.js', '!bower_components/**' ], dest: 'build/' }] } }); }

And got a lot of weird output, which may point to some unused and unloved requires:

Total unused dependencies: 89 in 64 files.
Total processed files: 251

The output is actually pretty long, so I'll file a separate bug which we can z-never it.

UPDATE: Filed as #2392

FTR p is now used.

shane-tomlinson · 2015-05-06T20:14:24Z

app/scripts/views/permissions.js

+  'stache!templates/permissions',
+  'lib/promise',
+  'lib/auth-errors',
+  'lib/constants',


I don't see Constants used anywhere either.

shane-tomlinson · 2015-05-06T20:15:36Z

app/scripts/views/permissions.js

+      var self = this;
+      var account = self.getAccount();
+
+      self.logScreenEvent('proceed');


As part of this PR, can you update the client-metrics doc?

shane-tomlinson · 2015-05-07T15:43:23Z

@zaach - can you rebase this?

shane-tomlinson · 2015-05-10T09:41:59Z

@zaach - this PR is epic! It's looking pretty good, mostly small nits at this point. I have to test it pretty thoroughly, I see you have already added functional tests. Have you tested the screen in mobile to ensure it looks good?

shane-tomlinson · 2015-05-10T19:12:36Z

app/scripts/models/account.js

@@ -27,7 +27,8 @@ define([
    verified: undefined,
    profileImageUrl: undefined,
    profileImageId: undefined,
-    lastLogin: undefined
+    lastLogin: undefined,
+    grantedPermissions: Object.create(null)


A silly question, why Object.create(null) instead of a {}?

shane-tomlinson · 2015-05-10T19:14:57Z

app/scripts/views/permissions.js

+      self.logScreenEvent('proceed');
+
+      return p().then(function () {
+        account.saveGrantedPermissions(self.relier.get('clientId'), self.relier.get('scope'));


@zaach - I see the granted permissions are saved, yet when I go back through 321done's signin flow, I am re-asked for the permissions. Expected?

Bah– so currently when a user signs in we create a new account object and store it in localStorage, overwriting the existing data (including saved permissions). We'll have to merge in the old account info before storing it.

shane-tomlinson · 2015-05-10T19:17:38Z

app/tests/spec/models/auth_brokers/oauth.js

+        assert.isTrue(relier.isTrusted.called);
+      });
+
+      it('should prompt when relier is untrusted and needs scopes', function () {


What if the scopes match the already granted permissions?

shane-tomlinson · 2015-05-10T19:19:20Z

app/tests/spec/models/auth_brokers/oauth.js

+          return false;
+        });
+        sinon.stub(account, 'hasGrantedPermissions', function () {
+          return false;


Can you add a test case for true? I am seeing odd behavior in that I am being re-asked for permissions on 321done after I have already granted permissions. Is that the desired behavior? It feels a bit odd.

shane-tomlinson · 2015-05-10T19:20:41Z

app/tests/spec/views/force_auth.js

-          });
-          sinon.stub(view.fxaClient, 'recoveryEmailStatus', function () {
-            return p.reject(assert.fail);
+          sinon.stub(user, 'signInAccount', function (account) {


Less code is a beautiful thing.

shane-tomlinson · 2015-05-10T19:30:40Z

@zaach - when local testing, I am re-asked for permissions for 321done even though I have granted them. If that is expected, no problem, but it feels odd.

Here is what the screen looks like in both desktop and mobile:

Desktop

Mobile

Maybe tighten up the space between the "Proceed" and "Cancel" buttons? @johngruen and @ryanfeeley?

shane-tomlinson · 2015-05-12T20:37:16Z

app/scripts/models/account.js

-    // returns true if all attributes within ALLOWED_KEYS are undefined
-    isEmpty: function () {
+    // returns true if all attributes within ALLOWED_KEYS are defaults
+    isDefault: function () {


shane-tomlinson · 2015-05-12T21:24:45Z

app/scripts/models/user.js

+          var oldAccount = self.getAccountByUid(account.get('uid'));
+          if (! oldAccount.isDefault()) {
+            // allow new account attributes to override old ones
+            oldAccount.set(_.omit(account.attributes, function (val) {


Can you add a note here that this is what allows permissions to remain persisted per relier across sign ins? I know my own memory and am afraid I'll forget in 6 months.

Fix #2346.

shane-tomlinson · 2015-05-13T11:02:07Z

@zaach - I've been reviewing & testing this PR all morning, it looks really good! Well done!

I have a couple of small changes that I'd like to see that I will probably make myself and then merge:

auth_brokers->shouldPromptForPermissions doesn't actually use any info from the brokers and feels like it has feature envy of either the relier or the account. I'm going to move this function to the relier.
update the client events document to specify the full screen name for the events.
Two additional functional tests: signup->signin - no permissions should be requested. signin->signin - no permissions should be requested.
A long email address can overflow the container.

For the longer term, I think we should reconsider how we deal with account management when using multiple accounts. For example, if I sign in as user X, then sign in as user Y, then re-sign in as user X, I have to re-grant permissions.

* Move brokers->shouldPromptForPermissions to reliers->accountNeedsPermissions * Ensure long email addresses wrap in the permissions page. * Add the screen name to the event names in the permissions screen. * Add functional tests to ensure re-signin after both signup and signin work as expected.

coveralls · 2015-05-13T12:38:46Z

Coverage increased (+0.04%) to 97.91% when pulling df3bb96 on issue-2346 into b0d96f8 on master.

shane-tomlinson · 2015-05-13T12:42:51Z

For follow on - make the permissions screen generic. The permissions screen is currently hard coded to display the email permission.

@zaach

feat(oauth): show permission screen for untrusted reliers @zaach - this PR is awesome. Thank you for working through it with me, and for the cleanup you did along the way. r=@shane-tomlinson

ryanfeeley · 2015-05-13T17:38:07Z

Man, the more I look at this, the more I think we should have gone with the original design of a read-only email field. Thoughts @shane-tomlinson @zaach? Should I just file a bug?

shane-tomlinson · 2015-05-13T17:39:54Z

Should I just file a bug?

File it @ryanfeeley!

zaach · 2015-05-13T18:15:36Z

For example, if I sign in as user X, then sign in as user Y, then re-sign in as user X, I have to re-grant permissions.

Do you mean when the user clicks "Use different account"? In that case, yes, all localStorage is wiped out and you have to re-grant permissions. Our long term solution for this is the full-fledged account chooser screen #1844.

shane-tomlinson · 2015-05-13T21:00:41Z

Do you mean when the user clicks "Use different account"? In that case, yes, all localStorage is wiped out and you have to re-grant permissions. Our long term solution for this is the full-fledged account chooser screen #1844.

Yeah, the same also happens if an OAuth relier sends a user to signin with an email query parameter other than the currently signed in user.

zaach · 2015-05-13T21:12:10Z

Hmm, in that case we show the email form field but the cached credentials should still be intact. The only place I see where we clear cached credentials is here: https://github.com/mozilla/fxa-content-server/blob/master/app/scripts/views/sign_in.js#L208

ryanfeeley · 2015-05-13T22:11:06Z

Progress so far (showing editable Full name field for demo purposes only)
Button should not be blue (as full name cannot be empty in this example)

ryanfeeley · 2015-05-14T20:27:01Z

Latest @zaach @vladikoff @johngruen @pdehaan @jrgm

vladikoff · 2015-05-14T20:46:13Z

@ryanfeeley new bug #2423

zaach added WIP waffle:needs review and removed WIP labels May 5, 2015

shane-tomlinson reviewed May 5, 2015
View changes

zaach force-pushed the issue-2346 branch from 54493dc to efdf3c3 May 5, 2015

vladikoff reviewed May 5, 2015
View changes

app/scripts/views/permissions.js

],

function (Cocktail, _, FormView, BaseView, Template, p, AuthErrors, Constants,

URL, ResendMixin, BackMixin, ServiceMixin) {

//var t = BaseView.t;

This comment has been minimized.

Sign in to view

vladikoff May 5, 2015
Contributor

extra comment

pdehaan reviewed May 5, 2015
View changes

zaach force-pushed the issue-2346 branch from efdf3c3 to 01aecff May 6, 2015

vladikoff added this to the train 37 milestone May 6, 2015

shane-tomlinson reviewed May 6, 2015
View changes

app/scripts/models/account.js

@@ -174,6 +175,54 @@ define([

.then(function () {

return profileImage;

});

},

signIn: function (relier) {

This comment has been minimized.

Sign in to view

shane-tomlinson May 6, 2015
Member

I totally dig this.

shane-tomlinson reviewed May 6, 2015
View changes

app/scripts/templates/permissions.mustache

</div>

<div class="button-row">

<a id="proceed" class="button" href="#">{{#t}}Proceed{{/t}}</a>

This comment has been minimized.

Sign in to view

shane-tomlinson May 6, 2015
Member

Not a normal submit button like the other templates?

shane-tomlinson reviewed May 6, 2015
View changes

app/scripts/views/permissions.js

onSignUpSuccess: function (account) {

var self = this;

if (account.get('verified')) {

This comment has been minimized.

Sign in to view

shane-tomlinson May 6, 2015
Member

We so need an external state machine and to rip this stuff out of the views.

shane-tomlinson reviewed May 6, 2015
View changes

app/scripts/views/sign_in.js

return self.user.setSignedInAccount(account)

.then(function () {

return account;

console.log('account???', account.get('sessionToken'), account);

This comment has been minimized.

Sign in to view

shane-tomlinson May 6, 2015
Member

console!

shane-tomlinson reviewed May 6, 2015
View changes

app/scripts/views/permissions.js

'stache!templates/permissions',

'lib/promise',

'lib/auth-errors',

'lib/constants',

This comment has been minimized.

Sign in to view

shane-tomlinson May 6, 2015
Member

I don't see Constants used anywhere either.

shane-tomlinson reviewed May 6, 2015
View changes

app/scripts/views/permissions.js

var self = this;

var account = self.getAccount();

self.logScreenEvent('proceed');

This comment has been minimized.

Sign in to view

shane-tomlinson May 6, 2015
Member

As part of this PR, can you update the client-metrics doc?

zaach force-pushed the issue-2346 branch from 01aecff to bd778da May 7, 2015

shane-tomlinson self-assigned this May 7, 2015

shane-tomlinson reviewed May 10, 2015
View changes

zaach force-pushed the issue-2346 branch 2 times, most recently from 4acb60a to 58a59bf May 12, 2015

shane-tomlinson reviewed May 12, 2015
View changes

feat(oauth): show permission screen for untrusted reliers

123821d

Fix #2346.

zaach force-pushed the issue-2346 branch from 58a59bf to 123821d May 12, 2015

shane-tomlinson merged commit a52d6fb into master May 13, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

continuous-integration/travis-ci/push The Travis CI build passed
Details

shane-tomlinson removed the waffle:needs review label May 13, 2015

shane-tomlinson deleted the issue-2346 branch May 13, 2015

vladikoff mentioned this pull request May 14, 2015

Update permission screen design #2423

Closed

mozilla / fxa-content-server Archived

feat(oauth): show permission screen for untrusted reliers #2377

feat(oauth): show permission screen for untrusted reliers #2377

Conversation

zaach commented May 5, 2015

This comment has been minimized.

shane-tomlinson May 5, 2015 Member

This comment has been minimized.

vladikoff May 5, 2015 Contributor

This comment has been minimized.

pdehaan May 5, 2015 Contributor

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

This comment has been minimized.

zaach May 6, 2015 Author Contributor

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

This comment has been minimized.

zaach May 6, 2015 Author Contributor

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

shane-tomlinson commented May 6, 2015

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

This comment has been minimized.

pdehaan May 8, 2015 Contributor

This comment has been minimized.

zaach May 8, 2015 Author Contributor

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

This comment has been minimized.

shane-tomlinson May 6, 2015 Member

shane-tomlinson commented May 7, 2015

shane-tomlinson commented May 10, 2015

This comment has been minimized.

shane-tomlinson May 10, 2015 Member

This comment has been minimized.

shane-tomlinson May 10, 2015 Member

This comment has been minimized.

zaach May 12, 2015 Author Contributor

This comment has been minimized.

shane-tomlinson May 10, 2015 Member

This comment has been minimized.

shane-tomlinson May 10, 2015 Member

This comment has been minimized.

zaach May 12, 2015 Author Contributor

This comment has been minimized.

shane-tomlinson May 10, 2015 Member

shane-tomlinson commented May 10, 2015

Desktop

Mobile

This comment has been minimized.

shane-tomlinson May 12, 2015 Member

This comment has been minimized.

shane-tomlinson May 12, 2015 Member

shane-tomlinson commented May 13, 2015

shane-tomlinson May 5, 2015
Member

vladikoff May 5, 2015
Contributor

pdehaan May 5, 2015
Contributor

shane-tomlinson May 6, 2015
Member

zaach May 6, 2015
Author Contributor

shane-tomlinson May 6, 2015
Member

shane-tomlinson May 6, 2015
Member

shane-tomlinson May 6, 2015
Member

shane-tomlinson May 6, 2015
Member

shane-tomlinson May 6, 2015
Member

shane-tomlinson May 6, 2015
Member

shane-tomlinson May 6, 2015
Member

zaach May 6, 2015
Author Contributor

shane-tomlinson May 6, 2015
Member

shane-tomlinson May 6, 2015
Member

shane-tomlinson May 6, 2015
Member

shane-tomlinson May 6, 2015
Member

shane-tomlinson May 6, 2015
Member

shane-tomlinson May 6, 2015
Member

shane-tomlinson May 6, 2015
Member

shane-tomlinson May 6, 2015
Member

pdehaan May 8, 2015
Contributor

zaach May 8, 2015
Author Contributor

shane-tomlinson May 6, 2015
Member

shane-tomlinson May 6, 2015
Member

shane-tomlinson May 10, 2015
Member

shane-tomlinson May 10, 2015
Member

zaach May 12, 2015
Author Contributor

shane-tomlinson May 10, 2015
Member

shane-tomlinson May 10, 2015
Member

zaach May 12, 2015
Author Contributor

shane-tomlinson May 10, 2015
Member

shane-tomlinson May 12, 2015
Member

shane-tomlinson May 12, 2015
Member