enforce 2FA policy on removal of second factor and login

[stefan0xC]

after a user's 2FA is removed, they should be deleted from all organizations where the 2fA policy is enforced.

fixes #3798

[BlackDex]

Do we need to delete the user? Or can we revoke the user instead?

[stefan0xC]

Not sure if that is what Bitwarden does but I think we should be able to revoke the user instead. This will also revoke users instead of removing them when enabling the policy in the first place.

[BlackDex]

It does make it more easy to see for the admins and owners if a user is still part of the org, but just revoked.
The only thing that i didn't see when scrolling through the changes is, that is there a check done on the server side when a user is restored if 2FA is enforced? Not that an admin or owner is able to restore the user without that user having 2FA enabled?

Also, maybe we should not retrieve the Org data at all if the user should have 2FA enabled and the org requires it, instead of depending on the state of the users membership to an org?

[stefan0xC]

The only thing that i didn't see when scrolling through the changes is, that is there a check done on the server side when a user is restored if 2FA is enforced? Not that an admin or owner is able to restore the user without that user having 2FA enabled?

That check is already done in _restore_organization_user:

vaultwarden/src/api/core/organizations.rs

Lines 2162 to 2167 in 3d2df6c

	if user_org.atype < UserOrgType::Admin {
	match OrgPolicy::is_user_allowed(&user_org.user_uuid, org_id, false, conn).await {
	Ok(_) => {}
	Err(OrgPolicyErr::TwoFactorMissing) => {
	err!("You cannot restore this user because it has no two-step login method activated");
	}

Also, maybe we should not retrieve the Org data at all if the user should have 2FA enabled and the org requires it, instead of depending on the state of the users membership to an org?

Not sure I understand. Do you mean we should have a different method of enforcing the policy if we check for an organization? I.e. so we don't have to look up the org data multiple times for each member.

[stefan0xC]

Refactored the code so it's hopefully more efficient. @BlackDex I hope this is what you meant? Regarding the treatment of invited users I'm not sure what the best way to handle them is. I agree that revoking is definitely an improvement over removing a user. But I have to think about what happens to invited users if we don't make this exemption.

Checking for the member status only in enforce_2fa_policy_for_org():

• If a new account is invited to an organization with the 2FA policy enabled, joining that organization will fail for email invitees but it might succeed if mail is disabled. (Not sure about that but maybe we need to also enforce the policy on login?)
• If an existing account (no 2FA enabled) gets invited to an organization with the 2FA policy not enabled, they will not be revoked if the organization decides to enable the 2FA policy. If they try to join they will get the error message and know that they have to set up a 2FA provider.
• Only problem is a rare edge case: If an invited user enables 2FA and removes the 2FA themselves (or via /admin) before accepting the invitation. Then the user will be revoked and can't complete the invitation on their own, even if they enable 2FA again. And until they have done that, they can't be restored. (Which would be less of a problem if the users get removed)

Making an exception if a user is invited also in enforce_2fa_policy() should prevent that. I.e. no invitation will be revoked because they have not joined the organization. This should be always safe. (And it would not affect existing users where mail is disabled because they will be accepted automatically if their account exists. So maybe we need to add the OrgPolicy::is_user_allowed check to send_invite()?)

edit: Okay the check is done at _confirm_invite so it should be okay even when mail is disabled.

[BlackDex]

Refactored the code so it's hopefully more efficient. @BlackDex I hope this is what you meant? Regarding the treatment of invited users I'm not sure what the best way to handle them is. I agree that revoking is definitely an improvement over removing a user. But I have to think about what happens to invited users if we don't make this exemption.

Checking for the member status only in enforce_2fa_policy_for_org():

* If a new account is invited to an organization with the 2FA policy enabled, joining that organization will fail for email invitees but it might succeed if mail is disabled. (Not sure about that but maybe we need to also enforce the policy on login?)

The latter was what I meant. Check if a user has 2FA enabled if an org needs this, and if so, just not return that org in the sync list, not even if that user is a full member (Except if that member is an owner or admin of course).

* If an existing account (no 2FA enabled) gets invited to an organization with the 2FA policy not enabled, they will not be revoked if the organization decides to enable the 2FA policy. If they try to join they will get the error message and know that they have to set up a 2FA provider.

* Only problem is a rare edge case: If an invited user enables 2FA and removes the 2FA themselves (or via `/admin`) before accepting the invitation. Then the user will be revoked and can't complete the invitation on their own, even if they enable 2FA again. And until they have done that, they can't be restored. (Which would be less of a problem if the users get removed)

Making an exception if a user is invited also in enforce_2fa_policy() should prevent that. I.e. no invitation will be revoked because they have not joined the organization. This should be always safe. (And it would not affect existing users where mail is disabled because they will be accepted automatically if their account exists. So maybe we need to add the OrgPolicy::is_user_allowed check to send_invite()?)

I think the latter sounds sane and valid. That would make it user-friendly and safe i think.
I have not checked if you can see the 2FA status of a user in the revoked overview, but if that is the case, that would be nice.

[BlackDex]

Quick look at the code again, looks ok too me. Not yet tested it my self though.

[BlackDex]

I have not yet tested the code it self, but it looks ok on first sight.
Some small changes i think which could make it better :).

[stefan0xC]

I've removed a commit because UserOrganization::find_by_user_and_policy will already only return confirmed users

vaultwarden/src/db/models/organization.rs

Lines 740 to 742 in d722328

	.filter(
	users_organizations::status.eq(UserOrgStatus::Confirmed as i32)
	)

so the additional check is not necessary.

Maybe we could improve enforce_2fa_policy_for_org() to only affect confirmed users as well, since accepting/confirming will already prevent users from joining if the policy is active.

[tessus]

May I ask what the status is? I've tested this and it works.

[BlackDex]

I think overall it is ok, and workings also as far as i can tell.
Just needs a rebase a few small tweaks to make it more nice.

[BlackDex]

I think overall it is ok, and workings also as far as i can tell.
Just needs a rebase a few small tweaks to make it more nice.