863287 - Clearly define and document the LoginAPI

Status: RESOLVED FIXED

(Webmaker Graveyard :: Login, defect)

Description

[Kieran Sedgwick (:sedge)]

Attachment: Etherpad for Login API Definitions

I want a clear, crowd-approved definition of our Login API so that people can start incorporating it into other components.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

You probably want to cc the desired reviewers on this bug, and add a comment when you're ready to have them look it over.

Comment 2

[Kieran Sedgwick (:sedge)]

Comment on attachment 739084 [details]
Etherpad for Login API Definitions

I just completed my first draft of a working API that (mostly) makes sense to me.  It doesn't reflect the implementation just yet, but that's the next step once this is confirmed to be accurate.

Comment 3

[Jon Buckley]

Comment on attachment 739084 [details]
Etherpad for Login API Definitions

Notes:

* You don't need a verifiedEmail attribute; When you login to Persona for the first time, you must verify your email before the login process will succeed.

Questions:

* What's isSuspended for?
* Why is there an "error" attribute for each response? Something like "status": "okay" or "status": "failure" would make more sense

Comment 4

[Kieran Sedgwick (:sedge)]

:jbuck - Thanks for the feedback!  Here are my thoughts:

* You don't need a verifiedEmail attribute; When you login to Persona for the first time, you must verify your email before the login process will succeed.

// The API was conceived to allow (here it comes :P) multiple authentication types in future iterations. I couldn't think of a reason not to include it, if that actually is a possibility.

* What's isSuspended for?

// I had the idea to give both a suspend and delete option for a Webmaker profile, just in case a user wants the option to bring their Makes back.  This flag would indicate whether the profile is active or not.

* Why is there an "error" attribute for each response? Something like "status": "okay" or "status": "failure" would make more sense

// I was thinking that it might be more convenient this way - if the "error" attribute is non-falsy then there is an error, and the contents give the details! What do you think?

Comment 6

[Kieran Sedgwick (:sedge)]

For now I'm closing this bug, because it seems like we've committed to the approach taken in the attachment - at least for now.

Comment 7

[David Humphrey (:humph)]

Comment on attachment 739084 [details]
Etherpad for Login API Definitions

You probably want to do this in the README.md file vs. an etherpad.

Comments:

* What do we expect _id to be?  Is it an email?

* Since you only have one kind of name, `displayName` can probably just be `name`.  What sort of restrictions are we going to place on the name?  There are probably some hard limits we want to impose on length, valid characters, etc.  You should speak with Jon/JP.

* Probably obvious, but the default value for `subdomain` will be the name with spaces removed, perhaps other characters.  Same question about limits on this one, perhaps more so here.

* The verifiedEmail field, do we still need this?  If your email address is Persona, it's already verified.

* One thing I think is missing in this model is any notion of opting in/out of emails from us.  I know they want to have some way to send people emails, and let them opt out.  I'm unclear if a single boolean is enough, or if it's more granular.  Probably you should talk to Erica for her opinion, and maybe Ryan.

* POST /user - You mention that email takes { String | Persona Email }.  Why not require a Persona email?  We need an email that's valid so we can mail people.

* PUT /user/:id - What are the implications for a subdomain being changed after the account is created and there is content already there?  You should discuss with JP/Jon.

* A lot of these actions on a user's account should probably trigger us to send verification/info emails to the user.  For example, if someone changes some value of my account info, I should get an email telling me it happened, and letting me click a link to undo it.  We might also want to do this for delete, where delete isn't actually done until someone OKs it via email.  This probably needs new bug(s).

Another question: do we want to provide an end-point for sending a user an email from us?  For example, if one of our tools needs to email a user, should login.w.o handle this for them?

Comment 8

[Ross Bruniges [:boozeniges]]

Comments above should be confirmed - then we can re-resolve

Comment 9

[Kieran Sedgwick (:sedge)]

:humph - Back to the drawing board! Here are my thoughts:
----------------------------------------------------------

You probably want to do this in the README.md file vs. an etherpad.

// My thinking was to encourage collaboration around the topic, but I'm happy to make the switch

Comments:

* What do we expect _id to be?  Is it an email?

// Email. Semantically we have them separate, but Mongoose requires an "_id" field.

* Since you only have one kind of name, `displayName` can probably just be `name`.  What sort of restrictions are we going to place on the name?  There are probably some hard limits we want to impose on length, valid characters, etc.  You should speak with Jon/JP.

// "Name" it is! We decided to stick with alphanumeric plus "-" and "_", and I'll implement a hard limit of 20 characters

* POST /user - You mention that email takes { String | Persona Email }.  Why not require a Persona email?  We need an email that's valid so we can mail people.

// We decided today that a Persona account is required, therefore the only email that can be provided is a Persona email

Comment 10

[Kieran Sedgwick (:sedge)]

* One thing I think is missing in this model is any notion of opting in/out of emails from us.  I know they want to have some way to send people emails, and let them opt out.  I'm unclear if a single boolean is enough, or if it's more granular.  Probably you should talk to Erica for her opinion, and maybe Ryan.

// I'll flag a needinfo so they can get a look at this.  Erica, Ryan, see this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=864406

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

We'd likely want the ability for the data model to support multiple types of email opt in/out, though we may not expose things at that granular level to users.

(Standard practise is to allow opt out for notifications and engagement emails separately)

Comment 12

[Kieran Sedgwick (:sedge)]

Comment on attachment 739084 [details]
Etherpad for Login API Definitions

I've updated the document according to our discussions on the user model and API requirements.

:jbuck - You mentioned a problem you had with my "error" fields in the return from the API calls.  Did you see my response?  Do you have any thoughts on that?

Comment 13

[Jon Buckley]

Comment on attachment 739084 [details]
Etherpad for Login API Definitions

Yeah, that's fine. Lets roll with this

Comment 14

[David Humphrey (:humph)]

Comment on attachment 739084 [details]
Etherpad for Login API Definitions

I think `notifications` and `engagements` are poorly named (does having it TRUE mean you want or don't want?).  I'd suggest something like `sendNotifications` and `sendEngagements` or something.

My previous comments about updating the user's details still stand.  I'm unclear whether we should allow this or not for email and domain, and if we do, what that means for other parts of the system.

r+ with those issues somehow dealt with.

Comment 15

[Kieran Sedgwick (:sedge)]

:humph - I agree about "notifications" and "engagements".  I'll make the change.  As for the email stuff, I think we should discuss it on this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=864406