Skip to content
This repository has been archived by the owner on May 10, 2019. It is now read-only.

Implement a Great User Experience as email providers implement persona #2797

Merged
merged 132 commits into from Dec 5, 2012

Conversation

lloyd
Copy link
Contributor

@lloyd lloyd commented Nov 29, 2012

NOTE: This pull request is not ready for merge, but is hungry for review.

In order to turn on bigtent and to prepare for popular email providers to implement persona support, these changes attempt to make UX great for users with email addresses that gain (or loose) "primary" persona support. Language and screens are added, and more.

10k Foot Overview

This change-set:

  • Introduces new screens and new language for all transition cases (still in progress) to help users understand what's going on.
  • Adds mechanisms to allow for zero-downtime switch flipping for primaries
  • Removes much state from the frontend and letting the server manage it

API Changes (@francois and @zaach, please review?)

  • list_emails now returns a list of emails without any additional information
  • address_info now returns a "state" for emails, that tells the frontend what flow it should display to the user
  • address_info now returns the current issuer for the domain
  • update_password is now callable by a assertion authenticated user (does not require pw auth)
  • used_address_as_primary is added, which is a means for the frontend to tell the backend that the user has used an address as a primary (causing a transition UX to not be displayed subsequently)
  • session_context now has a has_password key which the frontend can use to know definitively whether the user has set a password

Front End Work: (@shane-tomlinson, please review?)

  • several new screens added
  • we no longer store email type nor verified in local storage, everywhere where we were using these from local storage, we are now getting them from address_info
  • address_info is called Every Time the user picks an email address, which is the point where we determine what state to transition to
  • everywhere where we determined if the user has a password by looking for a local secondary address, we now defer to session_context's has_password property

Database Changes (@warner and @gene1wood, please review?)

  • lastUsedAs field is added, type field is removed. they both contain either primary or secondary. the difference is for type it was not expected to change, while lastUsedAs is the last form the email address was in when the user used it. lastUsedAs may be seeded from type for the purposes of database migration
  • idp table added. This holds recently seen idps and a timestamp of when they were last seen. This allows us to consider an idp broken when they stop serving .well-known without an explicit signal.

Security Questions (@warner and @francois please review?)

  • new information leaks in address_info
  • loosing of require auth level in update_password

Spec changes (@callahad and @warner and @benadida, please review?)

  • new parsing of .well-known documents
  • addition of disabled property in these documents, which is a signal to the implementation that it should fall back to secondary and the primary wishes to opt out
  • new SHOULD behavior of verifier: it should verify assertions whenever public-key is defined, but ignore disabled

lloyd and others added 30 commits November 7, 2012 15:52
…s used and in the jenkins env we don't npm install the lower level
… API on dbwriter for updating this from the browserid process
…t stepping back when an IdP we proxy stands up their own persona support
…rowserid and dbwriter listening for that event.
  * improve parsing of well-known documents and error messages
  * implement support for support:false - let idp's opt out issue #2697
  * expect that bigtent will issue certs with an issuer of 'login.persona.org'
  * improve check_primary_support script
  * add lots of comments and documentation to primary.js
…sona will delegate to bigtent domains using domain in get data
…roxy domains private key, not the browserid private key
First stab at updating cert_key and adding complete_transition wsapis
if (!emailKnown) {
r.state = "unknown";
} else if (r.type === 'secondary' && primarySeenRecently) {
r.state = "offline";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a race condition here that can lead to erroneously reporting "offline" when a domain explicitly disabled primary support.

Since primary.checkSupport and db.getIDPLastSeen are async, couldn't db.getIDPLastSeen first report that an IdP was seen recently, while primary.checkSupport finishes later, notices an {"enabled": false}, and reports a type of "secondary"?

In this case, the user will be shown the "offline" screen, even though the domain has explicitly disabled primary support and asked to use the fallback.

Edit: Here's a flow chart that outlines what we're doing / want to be doing, as best as I can tell.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's even weirder than that, since the call to checkSupport triggers an idp_seen event, which (eventually) updates the table that feeds db.getIDPLastSeen. So maybe two races.

Your flowchart looks right to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems we need check support to return a signal if the IdP is explicitly disabled. the test becomes (r.type === 'secondary' && primarySeenRecently && !r.explicitlyDisabled).

@callahad nice find! does this sound reasonable?

@warner I can't find the race in your reasoning. If an idp is seen, r.type === 'primary', in which case primarySeenRecently doesn't come into play, so the fact that it's out of date doesn't matter (and it should get up to date in ms). Am I missing something?

@warner
Copy link
Contributor

warner commented Nov 30, 2012

Quick scan of DB changes look ok to me. You might want to call out the specific command you have in mind for the DB upgrade.

Information leaks in /api/address_info :

The new code reveals (to everybody, without authentication) whether an email address is attached to a persona account for all kinds of IDPs, whereas the old code only revealed this for secondary IDPs. The new code also reveals whether the (secondary) address has been verified or not (the old code required auth before showing this). I think that's ok.. my hunch is that this is information that attackers could have deduced already.

Does the new /api/address_info do a .well-known fetch every time? That feels like a lot of traffic. This ties into the conversation we started last month about DNS-like expiration times for support documents (i.e. how long we're allowed to cache the response). It also reveals the time of every attempted login to the IDP, which would be less information-leaky if we only checked once every hour or day or whatever.

Looser auth level for /api/update_password: the code seems to correctly check the old password before allowing the operation, so I think the new code is as safe as the old code.

New .well-known parsing: I'm slightly in favor of using disabled=true instead of supported=false. I like the refactoring, splitting out parse-well-known.js and cleaning up the recursion to include just one function. Yay!

I might be wrong, but it seems like primary.js and fetchWellKnown is going to signal an error when the IDP 404s the .well-known fetch. This should be handled correctly (the IDP is signalled as a secondary), but would also emit a bunch of error messages ("primary support is misconfigured") that may obscure actual errors. Should handleResponse or maybe handleProxyIDP do a cb(null, false) in response to a 404 instead of an error?

The verifier certainly needs to honor the "disabled" flag. This will have to be part of the #1501 overhaul, since we don't currently check with the IDP at all.

Minor nits:

  • lib/db/mysql.js line 25: typo "timestamp" not "timestame"
  • lib/wsapi/address_info.js wants to use an "AsyncAND" pattern, and looks like a good use case for Deferreds or Promises or a similar device

@seanmonstar
Copy link
Contributor

Does the new /api/address_info do a .well-known fetch every time? That feels like a lot of traffic.

We've discussed with @gene1wood about being able to cache this files locally for a short time period (30s?), and that /wsapi/list_emails would actually start the requests. Then address_info would hopefully be able to use a cached version. Part of the goal, though, was allowing an IdP to almost instantly turn off IdP support, so we couldn't cache for too long.

lloyd and others added 18 commits November 29, 2012 20:46
…med, consider the domain a non-primary (letting the normal broken behavior take over). tests these two things.
* db APIs continue to return a lastUsedAs property
* Avoids deployment complexity for little benefit
* Based on feedback from stomlinson and seanmonstar
* Fixes busted unit test
Conflicts:
	tests/add-email-with-assertion-test.js
@lloyd lloyd merged commit 08c8c39 into dev Dec 5, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants