raiseProvisioningFailure requires undocumented argument with mandated wording #2339

Closed
nmalkin opened this Issue Aug 16, 2012 · 13 comments

Projects

None yet

7 participants

@nmalkin
nmalkin commented Aug 16, 2012

I think I just discovered an issue with either our implementation of provisioning for IdPs, or with our documentation, or with me.

For background, when we load the provisioning page from the IdP, we ask them to check the user's identity and, if in cases where authentication is needed, call navigator.id.raiseProvisioningFailure().

In our example code, we call it like this: navigator.id.raiseProvisioningFailure('user is not authenticated as target user'), but our documentation doesn't say anything about the argument to the function.

Now take a look at this line in BrowserID code. It seems to be that we require the message to be user is not authenticated as target user.

This is supported by some experimental IdP code that I wrote (not on Github yet), where any provisioning-failure message not matching the one above results in the login process failing with the error, "Action: Checking Address Info."

Is there some deep reason for this? Is it somehow part of the protocol? If so, we should definitely update our documentation. Otherwise, I see no reason at all for the second part of that if-statement to be there.

(On a related note, why do we even have the error.msg field if we never use it?)

Note that this issue affects at least one of our IdPs: eyedee.me. In two places, it calls raiseProvisioningFailure with a "forbidden" message.
But hostedpersona seems to use the (single) correct message everywhere, so maybe @ozten knows something I don't.

Member
ozten commented Aug 16, 2012

+1 this bite me too

Member
ozten commented Aug 16, 2012
nmalkin commented Aug 16, 2012

(Repeating for internal linking:)

This issue affected someone on the mailing list, was filed as #1182, where @ozten discovered what I just did, and filed #1207, which is open but has had no discussion.

Owner

If so, we should definitely update our documentation.

Done.

Otherwise, I see no reason at all for the second part of that if-statement to be there.

Not being too familiar with core, it looks like we're trying to distinguish between two cases:

  • We can't continue provisioning because the user isn't authenticated.
  • We can't continue provisioning because of an unrelated error.

In the first case, we recover by showing the user the authentication page. In the second case, we show a hard failure.

If this understanding is correct, I'd propose keeping the existing functionality (backwards compatibility), but adding a new callback for explicitly signaling that the user needs to authenticate. Maybe navigator.id.raiseAuthenticationNeeded?

Also: It might be worth checking for something similar in the implementation of navigator.id.raiseAuthenticationFailure.

Member
warner commented Aug 27, 2012

I wouldn't even describe the "authentication needed" situation as an exception at all: it happens at least once per user per browser, and requires special handling, so it's a normal occurrence, and "exception" isn't the right word for it. As such, I wouldn't use the word "raise" in the API for it. Maybe have the provisioning frame terminate by calling something named navigator.id.authenticationNeeded()?

Owner
callahad commented Mar 6, 2013

Someone make a proposal and let's get this wart fixed.

Even if we further refine the API soon after Beta 2, I'd still be really happy if we fixed this one little nit right now.

Contributor
lloyd commented Mar 7, 2013

on it.

Contributor
lloyd commented Mar 7, 2013

read code, read proposals, I like what @warner suggests:

Proposed Immediate fix:

  1. introduce navigator.id.authenticationNeeded()
  2. for compat, make .raiseProvisioningFailure() with a string of user is not authenticated as target user equivalent to calling .authenticationNeeded()

Proposed future UX improvement:
3. If any other failure is raised during initial provisioning attempt, let's display a user facing error message "we're sorry, cannot log you in right now Try a different email address"

NOTE: I don't know what the failure mode actually is today for first-time prov failure, but I'll set that up.

Member

Can the API be simplified using node-esque syntax:

function checkActiveSession(email, done) {   
   // do some checking. True if the user has a session, false if not    
   done(err, true || false);
}

function generateCertificate(email, publicKey, certDuration, done) {   
    // generate and sign key server side   
   done(err, certificate);
}

navigator.id.provision(checkActiveSession, generateCertificate);
Owner
callahad commented Mar 7, 2013

@shane-tomlinson That looks like an awesome start on a post-beta-2 refinement :)

Contributor
lloyd commented Mar 7, 2013

Refined proposed immediate fix:

  1. any invocation of .raiseProvisioningFailure() during the first attempt (pre-auth) causes authentication url to be loaded
  2. any invocation of .raiseProvisioningFailure() during second attempt (post-auth) causes us to redirect back to email entry

theory is the IdP tells the user what the authentication problem is, and us displaying an error screen would only be confusing. This addresses the main API pain point in a backcompat way and should only mean we remove a couple lines of code.

Later there are tons of opportunities to improve the idp api.

@lloyd lloyd added a commit that referenced this issue Mar 7, 2013
@lloyd lloyd issue #2339 - don't require that an IdP supply a specific text string…
… to indicate that the user is not authenticated
4cf3d4b
Contributor
lloyd commented Mar 11, 2013

The above PR is minimalistic, makes the behavioral change mentioned above, and awaits the thoughts of a front-end hero.

@lloyd lloyd added a commit that referenced this issue Mar 26, 2013
@lloyd lloyd + Edwin Wong issue #2339 - don't require that an IdP supply a specific text string…
… to indicate that the user is not authenticated
67cbd3c
@6a68 6a68 added a commit to 6a68/browserid that referenced this issue Apr 1, 2013
@6a68 6a68 Revert "issue #2339 - don't require that an IdP supply a specific tex…
…t string to indicate that the user is not authenticated"

This reverts commit 67cbd3c.

Signed-off-by: Jared Hirsch <ohai@6a68.net>
8535143
Owner
6a68 commented Apr 3, 2013

fixed by #3081. closing

@6a68 6a68 closed this Apr 3, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment