Add OpenID Connect IDP #1839
Add OpenID Connect IDP #1839
Conversation
| from models.user import User | ||
|
|
||
| SCOPES: dict[str, str] = { | ||
| "openid": "Your anonymous account identifier", |
There was a problem hiding this comment.
I think it's pseudonymous, rather than anonymous?
There was a problem hiding this comment.
heh I originally had "pseudoanonymous" and was like.. that's not a word
| <p> | ||
| <strong>{{ grant.client.client_name }}</strong> | ||
| {% if grant.client.official %} | ||
| is an official EMF orga-run service. |
There was a problem hiding this comment.
For "official" clients we might want to consider just skipping the authorize flow. If it's within EMF there's not necessarily much point asking people to authorise use of their website account - e.g. we don't do it within the website.
We could split the "official" bit from the "implicit_grant" flow, though.
There was a problem hiding this comment.
yeah had the thought in passing.
We could split the "official" bit from the "implicit_grant" flow, though.
how do you mean?
| } | ||
| scopes = scope_to_list(scope) | ||
| if "email" in scopes: | ||
| info["email"] = user.email |
There was a problem hiding this comment.
We should probably also include email_verified here (which in our case should always be true, because we don't do passwords?)
There was a problem hiding this comment.
We also don't verify emails though. That would be a new feature.
There was a problem hiding this comment.
But we send people login links?
There was a problem hiding this comment.
Yep, and once they've logged in for the first time we could say their email is verified. But we don't currently verify emails before use. Maybe we should, in theory there's the potential for someone to submit a CFP with someone else's email including a nasty message.
There was a problem hiding this comment.
And I'm not sure what the point of including email_verified is when it's not explicitly verified and we don't currently have people asking to use it?
There was a problem hiding this comment.
Ah, I hadn't realised that you get logged in immediately on cfp submit/ticket purchase - IMO that should probably change. Would also have the benefit of preventing people from buying tickets under a mis-spelled email, though IDK in reality how much that happens?
And I'm not sure what the point of including email_verified is when it's not explicitly verified and we don't currently have people asking to use it?
It's a standard claim, so OIDC clients may well use it - and if it were as simple as "every account's email is verified by the fact that they've logged in" seems like there's no reason to not include it?
There was a problem hiding this comment.
Ah, I hadn't realised that you get logged in immediately on cfp submit/ticket purchase - IMO that should probably change. Would also have the benefit of preventing people from buying tickets under a mis-spelled email, though IDK in reality how much that happens?
Once or twice per event, and it's almost always an easy fix via support email.
Requiring a verified email to buy tickets is a big change which is basically equivalent to adding a pre-registration stage, which we don't want to do.
It might be good to handle email verified status better, but that's probably out of scope of this issue.
There was a problem hiding this comment.
Yeah, it would make ticket buying much more annoying I guess. I think it could be good to put CFP submission behind an email link though.
It might be good to handle email verified status better, but that's probably out of scope of this issue.
I don't think that's the case - I hadn't realised when initially working on this that an EMF website account didn't automatically necessitate that the email was verified. IMO we should either:
- Report
email_verifiedstatus via OIDC - Require email_verified for OIDC authorizations (i.e
email_verifiedwould always be True)
Both are fairly easy to implement. I think the latter is probably preferable as it means that we don't have to rely on (potentially hacked-together by attendees) client apps checking email_verified to avoid the possibility of people logging in to client apps with a spoofed email.
There was a problem hiding this comment.
Yep, agree the second option is less likely to fail open. And yeah it's not much work, we set it on when authenticating a user, and clear it if we ever change a user's email address (in theory we should transfer everything to a new user, but that's often harder than fixing the typo). Probably a good excuse to add an admin page for this.
| from models.oidc import OAuth2AuthorizationCode, OAuth2Client, OAuth2Token | ||
| from models.user import User | ||
|
|
||
| SCOPES: dict[str, str] = { |
There was a problem hiding this comment.
It would be nice to have a profile scope as well, returning user.name as preferred_username and name (I assume we have Names for everyone, not just speakers?)
There was a problem hiding this comment.
Technically we might also have data like the users gender and address, but i'm certain we shouldn't expose that data over openid unless someone explicitely requests to have that (and we decide that's a valid use case)
There was a problem hiding this comment.
I think name is fine, and I think we all agree that we shouldn't do the latter!
5 participants
Resolves #1754
todo: