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

verify email on registration by invite #2804

Merged
merged 1 commit into from
Oct 19, 2022

Conversation

stefan0xC
Copy link
Contributor

I think we can skip a verification step when a user registers via an invite link because they will only have the invite link if the email address is theirs and they should not be able register another email address with the link either.

@tessus
Copy link
Contributor

tessus commented Oct 9, 2022

What happens with the users that already signed up with an email invitation link, but haven't verified their email yet?
But maybe I misunderstood the code in this PR.

@BlackDex
Copy link
Collaborator

BlackDex commented Oct 9, 2022

What happens with the users that already signed up with an email invitation link, but haven't verified their email yet? But maybe I misunderstood the code in this PR.

They need to verify there mail.
And if they start that procedure with smtp disabled, they can enter any code.

New users will automatically be verified with this patch when they join via an invite link.

@tessus
Copy link
Contributor

tessus commented Oct 9, 2022

I see, thanks. At least I read the code right. Cheers.

src/api/core/accounts.rs Outdated Show resolved Hide resolved
@stefan0xC
Copy link
Contributor Author

stefan0xC commented Oct 10, 2022

What happens with the users that already signed up with an email invitation link, but haven't verified their email yet?

My change would just affect people that have not registered yet as that is what I was wondering when I first started using Vaultwarden. Already existing users will still have to verify their email address the normal way. And also if/once #2799 is merged, when an invited user does not use the link with the invitation token but registers their address directly they would also have to separately verify their email.

I don't think that verifying this in other places, e.g. while accepting an invitation makes as much sense to me and I am not sure if this is even feasible because e.g. if SIGNUPS_VERIFY=true I cannot accept the invitation without having verified the email address in the first place.

Since the emergency access invitation is handled differently in the registration process it cannot be used to verify an email address the same way either unfortunately.

@stefan0xC
Copy link
Contributor Author

After thinking about it some more I'm now pretty certain that a separate verification does not make sense to me because email verification itself is just to make sure the mail is correct and the user can be reached, so they can get password hints and other emails like the recover-delete link (or incomplete 2fa warnings) not to ensure the identity of a user. For that there exists the account's fingerprint phrase which you need to (or at least should) check before confirming an user (in case you mistyped the email address or invited the wrong person).

It would certainly make life easier for new users (and myself as owner of an organization) when you have SIGNUPS_VERIFY=true, especially if you have also SIGNUPS_ALLOWED=false since I can just invite them via my organization (or the admin panel) and after they have signed up, I can confirm them without an another unecessary round of "now look into your inbox again and click yet another link so you can finally login".

@BlackDex
Copy link
Collaborator

I just tested a self-hosted environment from Bitwarden, and there it doesn't work like this.
If i register a new user via a link i still have to verify it afterwards though.

So, I'm not really sure we should change that flow on Vaultwarden though.

@stefan0xC
Copy link
Contributor Author

stefan0xC commented Oct 15, 2022

@BlackDex Okay, i've added a configuration option for people to opt-in, so this change won't affect anyone not expecting it. As to differences in the signup flow. Vaultwarden is already more configurable than Bitwarden server which essentially only has the option to globalSettings__disableUserRegistration completely, while we also have the options e.g. to disallow invites, require verification before login, etc. (I might be wrong about this as I am not that familiar with Bitwarden server configuration.)

@jjlin
Copy link
Contributor

jjlin commented Oct 15, 2022

Arguably, implicit email verification is how upstream Bitwarden should have done things in the first place. I don't think the goal of Vaultwarden should be making a "bug-for-bug" clone of Bitwarden, especially in areas where it wouldn't really hurt anyone to do things differently and better.

@BlackDex
Copy link
Collaborator

Arguably, implicit email verification is how upstream Bitwarden should have done things in the first place. I don't think the goal of Vaultwarden should be making a "bug-for-bug" clone of Bitwarden, especially in areas where it wouldn't really hurt anyone to do things differently and better.

As mentioned, I said I'm not sure what to do exactly in this case. Also you probably don't know if there is a good reason for this flow as neither do I. It could indeed well have been an oversight.

I'm trying to find a devil here to question pro's and con's for why or why not to have this enabled.

@stefan0xC
Copy link
Contributor Author

stefan0xC commented Oct 15, 2022

Yeah, no problem. I will lay out my thought process further so maybe you'll find a flaw in my reasoning or an issue I have overlooked.

I think the main difference is that Bitwarden has a fundamentally different registration model where you can sign up without even verifying the email address but if you want to have access to more features (like organizations) you need to verify your email address so you can get email invites. Whereas with Vaultwarden you have the SIGNUPS_VERIFY option where email verification is an earlier necessity in case it is enabled. This leads to the problem I have, that people get invited, signup and need to verify (as two separate steps), before they can login and accept the invitation (instead of making it more convenient by making the verification implicit with the signup via the invitation). In Bitwarden this signup flow makes more sense insofar that they can let users have almost the same experience as someone who has not been invited, because they can signup and then not accept the organization. (As @jjlin pointed out they could maybe skip this step too but maybe it's not possible because you can register with another email address in Bitwarden? I have not tested that but in Vaultwarden this is not possible.)

The only think I can think of that maybe a concern (if we ask the question what could go wrong with this) is that I get an invitation mail and I forward the mail with the Join link to someone else (e.g. to ask what this is) and they register an account in my name and have an account with my verified email. However, I don't think this will be a problem for most use cases (as people can delete their accounts if they have access to their email address and they will get notifications about logins and the welcome mail) and presumably people using a vaultwarden service know and have a way to contact the administrator.

As for the situation where someone has access to someone elses email account - e.g. the email provider or sysadmin - the email verification will not stop them, so I'd argue that this aspect should not be factored into consideration as we cannot prevent that.

But as I said before there is a separate mechanism for verifying the identity of an user before confirming a user to an organization so no sensitive information is leaked. So in the worst case there is the possibility that I'll conform someone else instead of the person. But there are many steps that must go wrong for that and if organization owners confirm without verifying the fingerprint phrase that's not something we can do anything about (as they could invite the wrong person when they type in the wrong email address already).

If someone is concerned this is an issue or they want the same behavior as Bitwarden server (with the separation of getting an invitation, signing up and then joining the organization after verifying the mail address instead of directly joining an organization by signing up via the invitation link as most people probably would want), making this feature optional and an explicit choice should still be the solution.

@stefan0xC
Copy link
Contributor Author

stefan0xC commented Oct 15, 2022

maybe it's not possible because you can register with another email address in Bitwarden

Just tested it and you can signup with another email address on Bitwarden, but the token validation will fail (when you login immediately afterwards) so you cannot accept the organization invitation with that account, which makes somewhat sense to me. But like I said, we don't allow registration of a different email address via the invitation link anyway, so that should be fine if we make the registration process better fitted to our use cases. I'd be against copying Bitwardens behavior in this case. (As users can change their email address afterwards anyway if they don't want to use the email address they have been invited with.)

they can signup and then not accept the organization

Okay, to be more precise If they sign up with the correct mail address and login afterwards they automatically accept the invitation (because there is no email verification required), but they still need to verify their email address (and confirmation is also needed of course) before they get access to the organization. <- So if we enable verification on invites, the flow would be in a way more like Bitwardens behavior, because currently with SIGNUPS_VERIFY enabled you'd have to verify the email, before clicking the Join link again to accept an invitation. (As this mainly affects the flow with required verification, I'd be also fine to remove a separate configuration option and make it only dependent on CONFIG.signups_verify() instead.)

@BlackDex
Copy link
Collaborator

Having thought about this also, and with an open system where you can just register a verification would be needed.
That should still be the case with this change, because it will only auto-verify when using the link.

And, indeed someone, if invited to an org would still need to be approved to join afterwards, and that is only possible by clicking the link, because else they arn't able to join an org.

So, having things clear on this, I think this is fine to be implemented and good to have this documented here 😄
It might not be needed to have an extra option regarding this though. Since the whole flow and explanation here would make it clear enough i think.

Copy link
Collaborator

@BlackDex BlackDex left a comment

Choose a reason for hiding this comment

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

Approved by me with a side note that this could be done without a new config option.
I think that would be ok too.

@stefan0xC
Copy link
Contributor Author

Approved by me with a side note that this could be done without a new config option. I think that would be ok too.

I will make the change later today. 👍

src/api/core/accounts.rs Show resolved Hide resolved
src/api/core/accounts.rs Outdated Show resolved Hide resolved
if `SIGNUPS_VERIFY` is enabled new users that have been invited have
their onboarding flow interrupted because they have to first verify
their mail address before they can join an organization.

we can skip the extra verication of the email address when signing up
because a valid invitation token already means that the email address is
working and we don't allow invited users to signup with a different
address.

unfortunately, this is not possible with emergency access invitations
at the moment as they are handled differently.
@dani-garcia dani-garcia merged commit 720a046 into dani-garcia:main Oct 19, 2022
@stefan0xC stefan0xC deleted the verify-on-invite branch October 19, 2022 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants