Skip to content
This repository has been archived by the owner on Dec 11, 2022. It is now read-only.

Create a verify-email page #16

Open
janus-reith opened this issue Jan 10, 2020 · 10 comments
Open

Create a verify-email page #16

janus-reith opened this issue Jan 10, 2020 · 10 comments
Labels
enhancement For issues that describe a feature that needs to be added, changed, or removed, but is not a bug help wanted For issues that have a clear solution described and are not currently prioritized core team work

Comments

@janus-reith
Copy link

janus-reith commented Jan 10, 2020

When creating the new user Account, the API will create a Meteor-compatible secret as verification Token and stores it in the the users collection, similar to the default Meteor Accounts Behavior. It then sends a welcome Email, which has the default env REACTION_IDENTITY_PUBLIC_VERIFY_EMAIL_URL set to http://localhost:4100/#/verify-email/TOKEN.

That Route doesn't exists, create it. And maybe change the /#/ to /accounts/ like the other routes. Need to decide how to use this: The default Meteor way of calling Accounts.verifyEmail still works, but would only set the email to verified in the users collection.

  • Setting it here, then calling the API to set it for the Accounts collection would be prone to errors.
  • Keeping Track of the Verifications on the Meteor Server Side to then also write directly to the Accounts collection also seems like a step backwards, don't assign more responsibility to Meteor.
  • Neither doing a collection watch from the API side to keep track of changes seems optimal, nor does having to check both the users and Accountd Accountd collection for changes on Context Building wit an additional db query.

Maybe the better option would be to not use the native Accounts.verifyEmail and instead call a GraphQL Mutation, like done for the password Reset. That could ensure both users and Account entries are set correctly. Also makes sense, as the token Generation also happens in the API already.

@janus-reith
Copy link
Author

I implemented my last suggestion and created a GraphQL Mutation verifyAccountEmail in the API, and a verification page in reaction-identity that calls this mutation in an effect upon render.

The Mutation works similar to the verifyEmail method in the Meteor codebase, with the difference that after uodating the user document it does the same for Account.

I first thought about using a MongoDB Transaction here to set both Account and user, but realized there is probably no point in doing that - If the Account is not present, the user documents email can still be set to validated and would be copied over to the account when that is created later.
But such a case would't even come up, as by now tokens are only issued upon account creation, so in regard to both considerations, there is no reason to keep them in sync if write to the latter fails.

Should I submit a PR for each identity and api, or does this need further discussion / a separate reaction-api issue?

@aldeed
Copy link
Contributor

aldeed commented Jan 21, 2020

@janus-reith Thanks for creating this issue. I was aware of this being missing but the verification flow has been a bit broken and untested even in 2.x, and the stock API does not check verification anywhere. That said, it is something we want to support. We just need to do it correctly.

Verify is similar to password reset in that it involves sending an email and the request initiates from an email link. I got password resets working, but I didn't get them 100% to where I wanted them. IMO currently too much is done by the API (namely, token generation). This means the API knows more about identity and users than it really should.

I haven't had time to vet it, but my thought was that both reset and verify should actually have most code moved to reaction-identity. Only the email sending would need API. If that is possible, then we would just need to add a generic sendEmail gql mutation on API and make sure identity has permission to call it. (This mutation would be useful in general, such as for admins to send test emails.)

Alternatively, if figuring out the authorization is too complex for now, the existing verify/reset mutations could be kept but changed to expect token generation is already done and token and user data for the email is passed in.

Hope this makes sense. If you have anything you want to PR in this direction, we can use that for further discussion. Otherwise this is currently on our post-3.0.0 project list.

@janus-reith
Copy link
Author

@aldeed The PR I meant to submit would just make use of the existing functionality in the api.
Currently the API already generates the token, and sends an email to the client with verification link.

That link just doesn't work currently, so basically what I did was:

  1. Create a Mutation in the API to actually deal with incoming tokens and set emails to verified, mostly based on a similar approach to the meteor-accounts codebase.
  2. Create a page in identity retrieving the token and calling the mutation.

Seems to be very stable so far.
That approach made the most sense for me, and you're right about that, it is very similar to the forgot password functionality so I followed that pattern.

I think I get the point regarding the splitted concerns, but I'm not sure I can follow how this should be approached on short term:
Would you want to strip out Account functionality from the API codebase and put it back into the meteor server of reaction-identity?

But sure, this can wait, it's just something that currently is broken in the beta release. Doing it this way seemed better than to strip out the welcome email functionality of the api.

@aldeed
Copy link
Contributor

aldeed commented Jan 22, 2020

@janus-reith Did you try handling the verification entirely on identity? I've been trying to use only vanilla Meteor Accounts for identity. So in theory the identity page can just call Accounts.verifyEmail with the token. In the callback, it should also call Meteor.logout() because we're trying to ensure that you're never actually logged in to Meteor on Identity URL for longer than you need to be.

The rest of what I was talking about was in regards to initiating a validation flow after the initial welcome.

@janus-reith
Copy link
Author

@aldeed I skipped using Accounts.verifyEmail altogether due to the considerations in the initial post:

The default Meteor way of calling Accounts.verifyEmail still works, but would only set the email to verified in the users collection

The mutation sitting in the API makes sure that the email is set to verified not only in users, but also in Accounts and works entirely with the token from the email. No signin in Identity should happen during the process.

@aldeed
Copy link
Contributor

aldeed commented Jan 23, 2020

I was afraid that might be true. I still think it would be best to use the Meteor API, and then call the API after that only for purposes of synchronizing the verified: true. We'd just have to figure out how to make that call secure.

The main goal is to have zero references to the users collection in the API codebase because users = identity, and we would like identity to be completely replaceable. As in, you should be able to use something like Auth0 for identity if you prefer. This may not be completely true in 3.0.0, but we're working toward that future in 3.x.

@aldeed
Copy link
Contributor

aldeed commented Jan 23, 2020

And I'll also look into whether tracking email addresses and their verification status on Account is even necessary. Ideally maybe that would even be a separate concept entirely. So rather than just "what is your email address?", the question would be "what email address(es) should receive notifications?", etc. For Reaction standpoint, we should only care about the latter type of question, and we may be even less concerned about verifying it (though support for verifying it is useful).

@janus-reith
Copy link
Author

janus-reith commented Jan 29, 2020

@aldeed Yes, that sounds reasonable so far. I like the more abstracted approach on this, but I’m not sure yet where to get started. The part that probably really needs to be dealt with first is, how the Login Provider/Meteor and the API could communicate without both accessing the same DB Data and without unnecessary added complexity. Maybe that solution could take a more general approach that could be used for other services aswell that might be separated at some point? Also, if GraphQL is the right fit for this, or if there might be something more barebone or more complex (More in the direction of some Messaqe Queue Maybe? Kafka although versatile would probably be a bit too complex to include in the general project setup)

@aldeed
Copy link
Contributor

aldeed commented Jan 30, 2020

@janus-reith Kafka will most likely be assumed to be part of the stack soon, though we're not quite there yet. Where we're at now (as of today actually), is that only the "authentication" API plugin is aware of and accesses the "users" collection. So by replacing that plugin you could link up a different identity provider.

But it still works by both accessing the same DB, which as you say maybe isn't ideal. Possibilities:

  • Whether GraphQL or REST, API could directly query Identity server over the internal network.
  • The OAuth "subject" could be an entire user document instead of just the ID. No query necessary other than the Hydra exchange.
  • Kafka, eventually

And in the other direction, Identity really only needs the API for sending emails and perhaps to sync email verification status, though there may be other ways to achieve that.

I'll be continuing to work through some of this prior to the final 3.0 release. Appreciate your ideas.

@aldeed aldeed added enhancement For issues that describe a feature that needs to be added, changed, or removed, but is not a bug help wanted For issues that have a clear solution described and are not currently prioritized core team work labels May 15, 2020
@bayareacoder
Copy link

bayareacoder commented Feb 10, 2021

@aldeed The PR I meant to submit would just make use of the existing functionality in the api.
Currently the API already generates the token, and sends an email to the client with verification link.

That link just doesn't work currently, so basically what I did was:

  1. Create a Mutation in the API to actually deal with incoming tokens and set emails to verified, mostly based on a similar approach to the meteor-accounts codebase.
  2. Create a page in identity retrieving the token and calling the mutation.

Seems to be very stable so far.
That approach made the most sense for me, and you're right about that, it is very similar to the forgot password functionality so I followed that pattern.

I think I get the point regarding the splitted concerns, but I'm not sure I can follow how this should be approached on short term:
Would you want to strip out Account functionality from the API codebase and put it back into the meteor server of reaction-identity?

But sure, this can wait, it's just something that currently is broken in the beta release. Doing it this way seemed better than to strip out the welcome email functionality of the api.

@janus-reith Would you mind sharing your implementation as I don't see a PR for this? This is still not addressed by the RC team after a year so your implementation will be the best there is at this time since you mention it works and is stable! Thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement For issues that describe a feature that needs to be added, changed, or removed, but is not a bug help wanted For issues that have a clear solution described and are not currently prioritized core team work
Projects
None yet
Development

No branches or pull requests

3 participants