Issue 2497 refactor to idp #3982

ozten · 2013-10-16T17:24:43Z

Background for pull request: Issue #2497 (comment)

Creating a pull request to try to get some frontend help.

Thanks to @shane-tomlinson for previously reviewing and getting it this far.

Known Issues:

siteName and other options aren't passed in from Desktop code, I'm investigating in gecko

Needs Frontend <3

rpinfo changes in dev broke this patch, minimal hardcoded changes were made to get it working again
- Need help doing this correctly

seanmonstar · 2013-10-16T18:02:06Z

fabulous! gets reviewing goggles

seanmonstar · 2013-10-16T18:02:35Z

lib/static/views.js

-      res.json({ 'public-key': publicKey.toSimpleObject() });
+      res.json({
+        'public-key': publicKey.toSimpleObject(),
+          'provisioning': '/provision',


indent is off

seanmonstar · 2013-10-16T18:16:35Z

resources/static/common/js/lib/hub.js

@@ -12,6 +12,7 @@ Hub = (function() {
      currID = 0;

  function on(message, callback, context) {
+    console.log('HUB ON ' + message);


reminder to kill these console.logs before merge

Ya, the version of Desktop codebase that this runs against has horrible dev tools, so I need these crutches.

shane-tomlinson · 2013-10-17T14:11:15Z

lib/static_resources.js

+  '/common/js/helpers.js',
+  '/common/js/gettext.js',
+  '/common/js/browser-support.js',
+  'common/js/enable_cookies_url.js',


is this supposed to have a / prepended?

shane-tomlinson · 2013-10-17T14:12:14Z

lib/wsapi/discovery.js

+// secondary addresses
+const HOSTNAME = urlparse(config.get('public_url')).host;
+
+try {


It seems like this try/catch could be generalized and placed into primary.js

my bad, secrets.js

This call is made from 7 sites in the code. Not all of them wrap in a try/catch.
I'd rather not do this change which is outside of the scope of this PR.

shane-tomlinson · 2013-10-17T14:15:09Z

lib/wsapi/discovery.js

+  var wellKnown;
+  primary.checkSupport(domain, function(err, r) {
+    if (!err && r && r.urls) {
+      wellKnown = {


Can both of these well known settings be extracted into functions?

Sure, what do you mean exactly? Something like this?

function prepareWellKnown(pubKey, provUrl, authUrl) { provUrl = provUrl || config.get('public_url') + '/provision'; ... return { "public-key": pubKey, provisioning: provUrl, ... } }

shane-tomlinson · 2013-10-17T14:16:34Z

resources/static/dialog/js/modules/idp_authentication.js

+      options = options || {};
+      var self = this;
+
+      window.resizeTo(700, 440); // Gross


resizeBy might give you better mileage, resizeTo includes browser chrome which can differ depending on toolbars.

shane-tomlinson · 2013-10-17T14:17:33Z

resources/static/dialog/js/modules/idp_authentication.js

+            //      params.origin = user.getOrigin();
+            var rpInfo = bid.Models.RpInfo.create({origin: 'http://192.168.186.138:10001'});
+            user.setRpInfo(rpInfo);
+            dialogHelpers.createUser.call(self, email, info.password, function(info) {


Can these be extracted into functions? The pyramid o' doom is getting awfully deep.

shane-tomlinson · 2013-10-17T14:18:34Z

resources/static/dialog/js/modules/idp_authentication.js

+            // TODO merge broke us - user.setRPInfo must now be called like in
+            // resources/static/dialog/js/modules/dialog.js line 130, 286
+            //      params.origin = user.getOrigin();
+            var rpInfo = bid.Models.RpInfo.create({origin: 'http://192.168.186.138:10001'});


ya, we need this info for the origin somehow. Are all the RP parameters passed here? If so, we'll need tos/pp, siteLogo and backgroundColor as well.

So... What we need is a way to get those parameters into this dialog.... We could drag over the channel stuff that allows you to call navigator.id.registerChannel, and then pass them in that way. We could use query strings. We could possibly stuff that info into local or sessionStorage? Indexeddb? Dunno what the best route forward is, would like to find out what capabilities you have.

Do you have a SHA for before and after the rpinfo stuff?

What are the inputs and were do they come from? We should probably solve this in content space, but natively is also an option.

shane-tomlinson · 2013-10-17T14:19:32Z

resources/static/dialog/js/modules/idp_authentication.js

+                    });
+                  }
+                },
+                function(err) {


I wanna kill the error rendering with a spoon.

shane-tomlinson · 2013-10-17T14:24:15Z

resources/static/dialog/js/modules/idp_authentication.js

+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+BrowserID.Modules.IdPAuthentication= (function() {


so, FWIU, this is a super mini state machine. With as yuck as the state machine is, we may want to use the generalized portion of it here if there is ever a point where the user can go back a screen within the IdP. I can help you with that if necessary.

I'm trying to do this, but not really sure what to do. Can you make a pull request off of issue-2497-refactor-to-idp with a sketch (or complete implementation) of what you mean?

OTOH, we should hold off on adding the state machine functionality until we have more direction. No need to add complexity where it's not yet needed.

shane-tomlinson · 2013-10-17T14:25:50Z

resources/views/authenticate_layout.ejs

+      </section>
+
+      <% } %>
+


I believe the error/waiting/delay sections will need to be added here to handle when those events occur.

shane-tomlinson · 2013-10-17T14:26:40Z

resources/views/provision.ejs

+  getJSON('/wsapi/session_context', function(session, text, xhr) {
+    if (session.authenticated) {
+      getJSON('/wsapi/list_emails', function(data, text, xhr) {
+        if (data.emails.indexOf(email) === -1) {


Is there ever a chance in hell we'll use this universally? If so, [].indexOf will break in IE8.

If IE8 implements persona natively and re-uses these flows, I'll eat my hat. :)

shane-tomlinson · 2013-10-17T14:27:35Z

resources/views/provision.ejs

+              },
+              error: function(xhr, status, err) {
+                // TODO: Handle errors
+                if (console && 'function' === typeof console.log) {


There is navigator.id.raiseProvisioningFailure I believe for this case.

shane-tomlinson · 2013-10-17T15:08:56Z

So... The devil hiding in the details, password resets and transition states. Are they going to be handled?

ozten · 2013-10-17T17:13:50Z

The devil hiding in the details, password resets and transition states. Are they going to be handled?

Can we land this and start getting UX feedback, before implementing those features?

ozten · 2013-10-17T21:55:00Z

Trying to get awsbox working, I've merged from dev, but got bitrot for the second time.

I was able to see that logging.js moved and fix that.

The other issue is that the dialog just comes up blank.

To unblock myself, I've copied over scripts/deploy.js and changed the dependency to awsbox@0.6.2... but we'll have to figure out what changed about the dialog code that has broken the initial screen display for the new /auth screen. Any ideas?

jaredhirsch · 2013-10-18T18:24:59Z

@ozten I'll do some git twiddling on Monday, see if I can cleanly apply your changes on top of current dev.

jaredhirsch · 2013-10-21T18:16:40Z

ok, looking now.

jaredhirsch · 2013-10-21T20:07:07Z

@ozten nice work so far!

I was able to cleanly cherry-pick all the changes onto current dev (except for the commits merging dev into this branch). My branch is at https://github.com/6a68/browserid/tree/issue-2497-idp-rebased-not-merged.

One tiny logging path change was needed to get tests running, that's on the HEAD of that branch, a7fd566.

The local RP seems fine to me on my branch, I can log in and out.

ozten · 2013-10-21T20:08:49Z

@6a68 is rocking the merge from dev to update this branch, you rule!

Please try to reproduce the blank dialog I was seeing.

You can download a Persona enabled Desktop build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/aking@mozilla.com-62351505bca9/

Set a couple preferences (about:config):

dom.identity.enabled = true
dom.identity.persona_fallback = http://127.0.0.1:10002
toolkit.identity.debug = true

Restart Firefox.

dom.identity.persona_fallback defaults to our new ephemeral server https://desktoppersona.personatest.org/ but you can change this to any environment.

Steps to reproduce:

Launch Firefox with a clean profile
Go to http://127.0.0.1:10001
Click Get an assertion
Enter foo@bar.com

Actual: empty dialog

Expected: prompt to set a password

jaredhirsch · 2013-10-21T21:11:58Z

@ozten happy to help :-)

I've got the special FF build, I toggled the settings you mentioned (actually tried using the desktoppersona fallback as well as localhost), but I'm not sure what to do next.

When I try to log into RPs, I click the persona button but nothing happens. Tried 123done, mozillians, nothing. Also not seeing errors in the JS console--maybe there's a desktop-specific console for the native code that I'm unaware of?

I'll poke a bit more at my branch and yours, see if I can at least get an empty dialog using your branch.

ozten · 2013-10-21T21:45:53Z

So in nightly if you go to about:config and search for identity does it look like this

Clicking get Assertion should look like this

jaredhirsch · 2013-10-21T22:37:04Z

It looks like the blank dialog error was caused by using hashes, but the recently-landed #3971 I think means we should be using querystring instead.

@ozten said this got him past the blank dialog problem in a local branch, I've updated my issue-2497-idp-rebased-not-merged branch with the fix (commit (9e8ed0f)[https://github.com/6a68/browserid/commit/9e8ed0f]).

jaredhirsch · 2013-10-21T22:38:50Z

@ozten i don't have the toolkit.identity.enabled pref at all, but I have the other three matching your screenshot.

The second image is broken for me.

ozten · 2013-10-21T22:45:05Z

Thanks Jared!

Using your patch, I see Error: Relay frame could not be found.

Thanks for continuing to dig for breaking changes from merging.

jaredhirsch · 2013-10-21T23:09:38Z

I have my current branch running on https://rebasedidp.personatest.org. Will pick this up again in the morning. 🍻

jaredhirsch · 2013-10-22T23:55:48Z

@ozten looks like this line probably shouldn't have that IP hard-coded. I'm not totally clear on where we can get the proper domain info in this context, though. any ideas?

jaredhirsch · 2013-10-23T00:11:00Z

heh, I caused the Relay Error: we look for the hash, not the querystring, when we setup the channel. Removing that, or changing that code to look for ?NATIVE as window.location.search, returns us to the mysterious empty dialog situation we had before. Continuing to dig.

* Create RpInfo before starting the modules. Hard coding the origin and rpName for now * Update IdpAuthentication module to pass RpInfo to the appropriate modules. * Start the check_registration module instead of calling user.waitForUserValidation since it already takes care of error messages.

ozten · 2013-11-07T15:01:44Z

@fmarier or @shane-tomlinson I've removed temporary hacks, console.log, etc. We should be good to merge as discussed.

General note: The Goldilocks API breaks the Native implementation, so use http://192.168.186.138:10001/watch.html instead of http://192.168.186.138:10001/

ozten · 2013-11-07T16:13:58Z

G R E E N

fmarier · 2013-11-07T16:42:21Z

resources/static/dialog/js/modules/idp_authentication.js

+        mediator.subscribe("new_user", newUserComplete(self));
+
+        var rpInfo = bid.Models.RpInfo.create({
+          origin: "http://127.0.0.1:10001",


Did you forget to take these out?

fmarier · 2013-11-10T13:04:01Z

r+ from me. makes Travis happy. tested by Shane and Austin.

Issue 2497 refactor to idp

ozten · 2013-11-10T13:05:09Z

All the man needed was a Guiness at the airport!

Thanks!

This allows the verifier to work-around a Persona bug that has been fixed (mozilla/persona#3982) but not yet deployed. Fixes mozilla#19

ozten mentioned this pull request Oct 16, 2013

login.persona.org should be a true IdP #2497

Closed

seanmonstar reviewed Oct 16, 2013
View changes

lib/static/views.js

res.json({ 'public-key': publicKey.toSimpleObject() });

res.json({

'public-key': publicKey.toSimpleObject(),

'provisioning': '/provision',

This comment has been minimized.

Sign in to view

seanmonstar Oct 16, 2013
Member

indent is off

seanmonstar reviewed Oct 16, 2013
View changes

shane-tomlinson reviewed Oct 17, 2013
View changes

lib/static_resources.js

'/common/js/helpers.js',

'/common/js/gettext.js',

'/common/js/browser-support.js',

'common/js/enable_cookies_url.js',

This comment has been minimized.

Sign in to view

shane-tomlinson Oct 17, 2013
Member

is this supposed to have a / prepended?

shane-tomlinson reviewed Oct 17, 2013
View changes

resources/static/dialog/js/modules/idp_authentication.js

});

}

},

function(err) {

This comment has been minimized.

Sign in to view

shane-tomlinson Oct 17, 2013
Member

I wanna kill the error rendering with a spoon.

shane-tomlinson reviewed Oct 17, 2013
View changes

resources/views/authenticate_layout.ejs

</section>

<% } %>

This comment has been minimized.

Sign in to view

shane-tomlinson Oct 17, 2013
Member

I believe the error/waiting/delay sections will need to be added here to handle when those events occur.

shane-tomlinson reviewed Oct 17, 2013
View changes

ozten and others added 18 commits Sep 18, 2013

Make wsapi/address_info provie auth and prov info

e806ea4

Created a Discovery web service for Desktop

76b7ae3

A couple gross hacks to get around bugs.

00e66d0

Minor cleanup

7dc2536

Tighten up authentication for new, registering users

7095759

jshint and travis - haters gonna hate

52b67e0

well known now returns auth and prov urls in well known

777dfef

Use cachify for idp_authentication resources

3f43f1d

cleanup - pass in email to authenticate module

c7a3f11

Switch from jquery to micrajax for /provision

959d6f1

Merged dev, which broke new user registration

e602d3f

Refactor to module

7d9173d

Temporary fix for rpinfo changes. Improve getErrorDialog

032bfd4

Fixes based on stomlinson and seanmonstar feedback

8a562be

Adding comments and renaming variable per francois

fe8f7ff

Just kidding, renamed a property of results

af245c1

Remove temporary hacks

e6bf257

fmarier reviewed Nov 7, 2013
View changes

Added test for disabled domain. Removed hardcoded rpInfo per fmarier

8b18afe

fmarier added a commit that referenced this pull request Nov 10, 2013

Merge pull request #3982 from mozilla/issue-2497-refactor-to-idp

173be4f

Issue 2497 refactor to idp

fmarier merged commit 173be4f into dev Nov 10, 2013
1 check passed

1 check passed

default The Travis CI build passed
Details

fmarier mentioned this pull request Nov 29, 2013

remove support for poorly behaved fallback mozilla/browserid-local-verify#5

Closed

fmarier mentioned this pull request Apr 9, 2014

Expose the allowURLOmission config option mozilla/browserid-verifier#32

Merged

jaredhirsch deleted the issue-2497-refactor-to-idp branch Apr 20, 2014

seanmonstar added a commit to seanmonstar/browserid-verifier that referenced this pull request May 30, 2014

Expose the allowURLOmission config option

c1e30ea

This allows the verifier to work-around a Persona bug that has been fixed (mozilla/persona#3982) but not yet deployed. Fixes mozilla#19

mozilla / persona Archived

Issue 2497 refactor to idp #3982

Issue 2497 refactor to idp #3982

Conversation

ozten commented Oct 16, 2013

seanmonstar commented Oct 16, 2013

This comment has been minimized.

seanmonstar Oct 16, 2013 Member

This comment has been minimized.

seanmonstar Oct 16, 2013 Member

This comment has been minimized.

ozten Oct 17, 2013 Author Member

This comment has been minimized.

shane-tomlinson Oct 17, 2013 Member

This comment has been minimized.

shane-tomlinson Oct 17, 2013 Member

This comment has been minimized.

shane-tomlinson Oct 17, 2013 Member

This comment has been minimized.

ozten Oct 17, 2013 Author Member

This comment has been minimized.

shane-tomlinson Oct 17, 2013 Member

This comment has been minimized.

ozten Oct 17, 2013 Author Member

This comment has been minimized.

shane-tomlinson Oct 17, 2013 Member

This comment has been minimized.

shane-tomlinson Oct 17, 2013 Member

This comment has been minimized.

shane-tomlinson Oct 17, 2013 Member

This comment has been minimized.

shane-tomlinson Oct 17, 2013 Member

This comment has been minimized.

ozten Oct 17, 2013 Author Member

This comment has been minimized.

shane-tomlinson Oct 17, 2013 Member

This comment has been minimized.

shane-tomlinson Oct 17, 2013 Member

This comment has been minimized.

ozten Oct 17, 2013 Author Member

This comment has been minimized.

shane-tomlinson Nov 7, 2013 Member

This comment has been minimized.

shane-tomlinson Oct 17, 2013 Member

This comment has been minimized.

shane-tomlinson Oct 17, 2013 Member

This comment has been minimized.

ozten Oct 17, 2013 Author Member

This comment has been minimized.

shane-tomlinson Oct 17, 2013 Member

shane-tomlinson commented Oct 17, 2013

ozten commented Oct 17, 2013

ozten commented Oct 17, 2013

jaredhirsch commented Oct 18, 2013

jaredhirsch commented Oct 21, 2013

jaredhirsch commented Oct 21, 2013

ozten commented Oct 21, 2013

jaredhirsch commented Oct 21, 2013

ozten commented Oct 21, 2013

jaredhirsch commented Oct 21, 2013

jaredhirsch commented Oct 21, 2013

ozten commented Oct 21, 2013

jaredhirsch commented Oct 21, 2013

jaredhirsch commented Oct 22, 2013

jaredhirsch commented Oct 23, 2013

ozten commented Nov 7, 2013

ozten commented Nov 7, 2013

This comment has been minimized.

fmarier Nov 7, 2013 Contributor

fmarier commented Nov 10, 2013

ozten commented Nov 10, 2013

seanmonstar Oct 16, 2013
Member

seanmonstar Oct 16, 2013
Member

ozten Oct 17, 2013
Author Member

shane-tomlinson Oct 17, 2013
Member

shane-tomlinson Oct 17, 2013
Member

shane-tomlinson Oct 17, 2013
Member

ozten Oct 17, 2013
Author Member

shane-tomlinson Oct 17, 2013
Member

ozten Oct 17, 2013
Author Member

shane-tomlinson Oct 17, 2013
Member

shane-tomlinson Oct 17, 2013
Member

shane-tomlinson Oct 17, 2013
Member

shane-tomlinson Oct 17, 2013
Member

ozten Oct 17, 2013
Author Member

shane-tomlinson Oct 17, 2013
Member

shane-tomlinson Oct 17, 2013
Member

ozten Oct 17, 2013
Author Member

shane-tomlinson Nov 7, 2013
Member

shane-tomlinson Oct 17, 2013
Member

shane-tomlinson Oct 17, 2013
Member

ozten Oct 17, 2013
Author Member

shane-tomlinson Oct 17, 2013
Member

fmarier Nov 7, 2013
Contributor