Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement email confirmation when USERNAME_IS_EMAIL=true #138

Open
martinreus opened this issue Oct 27, 2019 · 11 comments
Open

Implement email confirmation when USERNAME_IS_EMAIL=true #138

martinreus opened this issue Oct 27, 2019 · 11 comments

Comments

@martinreus
Copy link
Contributor

martinreus commented Oct 27, 2019

When signing up for an account, especially when USERNAME_IS_EMAIL is set to true, it would be nice to have an email confirmation flow, to prevent malicious "account injection". Implementation could be similar to how other platforms approach this, namely:

  1. User signs up
  2. Authn stores user info, but does not automatically allows him/her to log in
  3. Authn sends an email to the account holder with a one-time activation token
  4. When user clicks on the sent link, account is activated and he is allowed to log in (or is automatically logged in with the activation code).
@ppacher
Copy link

ppacher commented Oct 27, 2019

Hi,

I'm afraid but your request seems to be out-of-scope for that project and has been targeted at https://keratin.github.io/authn-server/#/guide-restrict_signups_by_domain and #124 (comment) as well as #70 (comment).

@cainlevy please correct me if I'm wrong. Otherwise maybe a dedicated write-up on keratin.github.io can be used to clarify on that?

@ppacher
Copy link

ppacher commented Oct 27, 2019

Ok, my bad I've overseen the discussion on #137 . Though I also believe that setting and maintaining SMTP / email verification in AuthN itself will dramatically increase complexity as people will then ask for a customizable implementation (custom email server, various authentication flows, ...). As @martinreus mentioned in the last comment, linking those accounts while being logged in may be enough so I'd advise not going down the road of SMTP handling and Mail templating.

If E-Mail verification from AuthN is still important I would rather add a new webhook to trigger and a private endpoint to confirm a mail address. This way the host application is still responsible of actually sending those verification message. Something like this may also be interesting for the discussions on #112 or #120

@martinreus
Copy link
Contributor Author

@ppacher thanks for the feedback! Yeah, definitely I would also say don't implement email and SMTP stuff in Authn. We can have a new webhook and let the host application handle sending this verification email.

@cainlevy
Copy link
Member

The primary challenge I see on this feature is the question of product requirements. I've tried to scope AuthN so that it only implements clear patterns, but I've seen too many patterns for email handling to know if there's a clear answer. For example:

  • An account may have many emails. Each may be confirmed, and any confirmed email may be promoted to primary.
  • An account may have only one email. The email may not be updated except with a newly confirmed replacement (delayed update).
  • An account may have only one email. The current email may or may not be confirmed.
  • An account may have only one email. The first email must be confirmed, but later ones aren't a concern.

Then there's the question of what an unconfirmed account is authorized to do. Can they do some less critical things, or are they blocked from proceeding until they finish?

A more hands-off implementation might instead implement a single accounts.confirmed boolean that the host application is expected to toggle. It's not email confirmation, but maybe it could unlock some interesting behavior? Whatever it accomplishes must be distinct from accounts.locked, and must be effective even if it's never exported back to the main application (otherwise it's sneaking authorization into the authentication system.)

@martinreus
Copy link
Contributor Author

martinreus commented Oct 28, 2019

Interesting patterns, especially when having more than one e-mail (I guess I've never seen this elsewhere?). When USERNAME_IS_EMAIL=true we have only one e-mail registered for an account in Authn, right? Is the multiple mail per account scenario also implemented in Authn?

accounts.confirmed seems to be a nice way to deal with scenarios where we might want to let the user sign in but also let the host application know that the account is not fully activated. I think that leaving this accounts.confirmed to be toggled by the host application however IMHO leaks authorization responsibilities into it, which I'm not sure would be a good thing..

I was imagining something like this ( sorry for the bad diagram 😅 )

image

I wrote "logged in?" with a question mark because I'm not sure if the user should be already logged in with an unconfirmed account (I believe we could let him already be logged in even in an unconfirmed state)

@ppacher
Copy link

ppacher commented Oct 28, 2019

Is the multiple mail per account scenario also implemented in Authn?

I'm not sure but IMHO the multi-mail scenario is not yet covered by authn.

I'm also pro account.confirmed but I'm not sure if the "verification" endpoint should be on authn rather than the host/main application. If we keep that endpoint on the main application it is also possible to use/verify accounts using, i.e. telephone numbers (and verifying by sending a SMS). The main application just needs to toggle that flag on the account and in the sense of authn I guess it's fine that the main app needs to handle authorization based on the confirmed flag (maybe we should include that into the JWT?).

If the verification endpoint is actually part of authn we would also need to think about how to redirect the user to the main application. The simplest way would be to specify that redirect target via an env variable and use a 301 Moved Permanently. However, if that target can depend on some other factor that the main app chooses (e.g. based on some invitation code) we may need to include that into the verification link (like ?redirect=http://app.example.com). Though, in that case we need to ensure to not open up a "open redirect" vulnerability and use some whitelist/pattern matching approach to verify the redirection target.

@martinreus
Copy link
Contributor Author

martinreus commented Oct 29, 2019

I'm not sure but IMHO the multi-mail scenario is not yet covered by authn.

Cool, I think this eases up implementations.
Regarding the flow where the host app is responsible for setting the account.confirmed to true: my only fear is that letting every developer implement this endpoint over and over, we risk having some of them inadvertently inserting security holes in their applications.

maybe we should include that into the JWT?

👍 here

As per the redirect, we could use the existing APP_DOMAINS for letting them redirect to only known domains..

@cainlevy
Copy link
Member

cainlevy commented Oct 29, 2019

Honestly I'm not ready for implementation details yet. Some combination of tokens and redirects should work, and there's precedent for both in existing AuthN features.

However, AuthN is currently not the source of truth for contact information or authorization data. It's difficult to imagine adding email verification to the project scope because it's difficult to find a perspective that does not include adding one or both of those new responsibilities, each of which come with new and competing features.

When I mentioned the scenarios earlier, my point was that applications may vary on:

  • what and how many details they need for contact information
  • what things a user is authorized to do before they've taken an extra step

Given that the right way to implement email verification depends on decisions that AuthN has not made, and given that the usual motivations for email verification are related to out of scope features, I'm not yet sure that it's a good fit.

Perhaps we need to go back to a problem statement. Currently the only issue related to AuthN's existing project scope is:

As a user with a confirmed account based on my email address,
when I try to later authenticate using a third-party system that shares the same email address
then I want to seamlessly link with my existing account
so that I do not accidentally create two accounts or find myself unable to log in

Maybe (maybe) the way to solve this is not with a full email verification feature, but a requirement that the host app can define what "confirmed" means and inform AuthN when it happens to be true.

I say maybe because I'd really hope to discover more value from a "confirmed" boolean before adding it to the API. Currently the motivation is an edge case with a viable workaround.

@AlexCuse
Copy link
Contributor

I agree that letting authz functionality bleed into authn would be the wrong move. Would it work to configure an endpoint in the app that authn can call to see if a user has sufficiently proven ownership of the account to allow this move @cainlevy ?

This would keep control of the rules with applications (eg apps that handle protected data may want to enact stricter verification measures). If no endpoint is configured it could fall back to existing behavior although if this is going to be an officially supported mechanism I'd lean towards requiring use of this endpoint for anyone wanting to allow binding an oauth login to an existing username/password account.

@cainlevy
Copy link
Member

an endpoint in the app that authn can call

Can you say more about when AuthN would call the endpoint?

I'd lean towards requiring use of this endpoint for anyone wanting to allow binding an oauth login to an existing username/password account

I'd also love to hear more thoughts on this. Maybe the question is whether, in the scenario where someone has an "unproven" AuthN account, we think it's more likely the true email owner has secured the identity provider account (e.g. Google, Microsoft) or the AuthN account.

@AlexCuse
Copy link
Contributor

I guess it would call the app somewhere around here with the linkableAccountID: https://github.com/keratin/authn-server/blob/main/app/services/identity_reconciler.go#L40-L53 - it seems like a non-zero value there is really all there is to the current behavior. Alternatively we could call it from the handler and verify the "linkability" of the account before passing it in as such.

I thought about that second point as well but I guess I tend to favor the identity provider as the source of truth. If an attacker has taken over the identity provider account and its used as the username/email for the authn account, they can pretty trivially reset the password if the authn-serviced app is their target and supports password resets. This raises the question of what to do if MFA is enabled on the account though too I suppose. I wonder if all roads are gonna lead back to a way to better message the cause of failure and steer users into a flow that lets them authenticate the existing account before binding it to the provider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants