feature: Support single organization policy #1991

domdomegg · 2021-09-24T16:01:17Z

This adds back-end support for the single organization policy.

This is listed on the wiki as a feature in scope for Vaultwarden that a contribution would be welcome for.

NB: this is not already done by #1955, despite that PR enabling the enum for it (it only implements the RequireSso policy).

williamdes · 2021-09-25T08:09:03Z

src/static/templates/email/send_single_org_removed_from_org.hbs

@@ -0,0 +1,7 @@
+Removed from {{{org_name}}}
+<!---------------->
+You have been removed from organization *{{org_name}}* because you a member of multiple organizations, but it requires you to be a member of only it.


The "it" part is unclear

jjlin

You should probably review the relevant upstream PRs:

Email notification:

https://github.com/bitwarden/server/blob/master/src/Core/MailTemplates/Handlebars/OrganizationUserRemovedForPolicySingleOrg.html.hbs

Error messages:

jjlin · 2021-09-26T09:11:49Z

src/api/core/organizations.rs

+/// enabled isn't allowed to create new organizations or join other organizations
+///
+/// Ref: https://bitwarden.com/help/article/policies/#single-organization
+fn enforce_single_organization_policy(user_uuid: &String, conn: &DbConn) -> EmptyResult {


Given that this function is called from a couple of places, it would probably be better located near the top of the file.

I ended up inlining this function - given it's really just one very simple if statement and an error message (that needs to change depending on context) that seems to be the more sensible way. Also saw this other PR review comment that suggested we prefer inlining very small functions.

jjlin · 2021-09-26T09:13:28Z

src/api/core/organizations.rs

@@ -102,6 +102,7 @@ fn create_organization(headers: Headers, data: JsonUpcase<OrgData>, conn: DbConn
    if !CONFIG.is_org_creation_allowed(&headers.user.email) {
        err!("User not allowed to create organizations")
    }
+    enforce_single_organization_policy(&headers.user.uuid, &conn)?;


This currently returns an error message about joining an org, not creating one.

Good catch - have updated the error message to match upstream.

jjlin · 2021-09-26T09:16:50Z

src/static/templates/email/send_single_org_removed_from_org.hbs

@@ -0,0 +1,7 @@
+Removed from {{{org_name}}}


Consider keeping these messages the same as upstream if possible. The message you have below is missing a word anyway ("because you are a member").

I've made this match upstream, seems like the sensible thing to do 👍

jjlin · 2021-09-26T09:29:17Z

src/api/core/organizations.rs

@@ -747,6 +748,8 @@ fn accept_invite(_org_id: String, _org_user_id: String, data: JsonUpcase<AcceptD
                    err!("You cannot join this organization until you enable two-step login on your user account.")
                }

+                enforce_single_organization_policy(&user_org.user_uuid, &conn)?;


Upstream actually does two checks in the accept flow:

If the org the user is joining has the single org policy enabled and the user isn't joining as an owner/admin, then reject the acceptance if the user is already a member of any other org. This check isn't implemented currently AFAICT.

Otherwise, if the user is an accepted or confirmed member of an org with the single org policy enabled, then reject the acceptance. This is what the current check does, except it only checks for confirmed users instead of accepted users as well. That's sort of an edge case and maybe we can let it slide for now, given that some of our other policy implementations would have the same issue.

Correct, I missed check 1 and have now implemented it. One thing that might be worth noting is that like upstream, I have included invited org users (checks allOrgUsers are the same org, and the query doesn't filter out invites).

On 2, yes this is an issue which is shared by many of the policy checks - I've renamed a couple methods to make it more explicit about what they're checking and added a TODO to check against accepted users too like upstream. I think this may be best in a separate PR as it'll probably require updating the way many of the policy checks are done.

Another thing to note is that because we auto-accept users when email is disabled without running through these checks, they are not enforced there. Again, this is a problem with all the checks but I'm not sure the most sensible way to go about resolving this?

AFAIK having no email service is not a configuration that's expected or designed for in upstream Bitwarden, so people who don't bother configuring it shouldn't expect things to work completely normally.

jjlin

I'd suggest rebasing and squashing at the end as well

jjlin · 2021-09-30T07:15:45Z

src/api/core/organizations.rs

+        let org_list = UserOrganization::find_by_org(&org_id, &conn);
+
+        for user_org in org_list.into_iter() {
+            if user_org.atype < UserOrgType::Admin {


This should also filter out users in Invited state: https://github.com/bitwarden/server/blob/dac3b3e8939504959d94c2e2fd13789723ab2d57/src/Core/Services/Implementations/PolicyService.cs#L88

Those users will just get the appropriate error message when they try to accept.

Have updated.

FYI I think the 2FA organization policy also misses this. I can raise a PR to fix that if you'd like?

vaultwarden: https://github.com/dani-garcia/vaultwarden/blob/main/src/api/core/organizations.rs#L1210
upstream (bitwarden/server): https://github.com/bitwarden/server/blob/dac3b3e8939504959d94c2e2fd13789723ab2d57/src/Core/Services/Implementations/PolicyService.cs#L93

The 2FA org policy isn't my code anyway, but if there's something that can be fixed or improved, feel free.

jjlin · 2021-09-30T07:30:07Z

src/api/core/organizations.rs

@@ -1219,6 +1248,29 @@ fn put_policy(
        }
    }

+    if pol_type_enum == OrgPolicyType::SingleOrg && data.enabled {


Some comments that summarize what this code is trying to do would be nice, but I think it would read more clearly with org_list renamed to org_members and user_org renamed to member.

Good feedback, have renamed variables as suggested and added explanatory comments

jjlin · 2021-09-30T07:35:14Z

src/api/core/organizations.rs

+            if user_org.atype < UserOrgType::Admin {
+                let is_member_of_another_org = UserOrganization::find_any_state_by_user(&user_org.user_uuid, &conn)
+                    .into_iter()
+                    .filter(|uo| uo.org_uuid != user_org.org_uuid)


This should also filter out users in Invited state: https://github.com/bitwarden/server/blob/dac3b3e8939504959d94c2e2fd13789723ab2d57/src/Core/Services/Implementations/PolicyService.cs#L112

Otherwise users will be dropped from this single org even if they didn't intend to join the other org they were invited to.

Good catch, fixed 👍

This adds back-end support for the [single organization policy](https://bitwarden.com/help/article/policies/#single-organization).

domdomegg · 2021-10-09T00:25:40Z

Thanks @jjlin for your detailed, timely and valuable feedback on this. And thanks @dani-garcia for merging 🙌

The single organization is now supported by vaultwarden, as per dani-garcia/vaultwarden#1991 We can therefore stop hiding the setting in the web UI.

williamdes reviewed Sep 25, 2021

View reviewed changes

jjlin suggested changes Sep 26, 2021

View reviewed changes

jjlin suggested changes Sep 30, 2021

View reviewed changes

feature: Support single organization policy

d014eed

This adds back-end support for the [single organization policy](https://bitwarden.com/help/article/policies/#single-organization).

domdomegg force-pushed the domdomegg/single-organization-policy branch from af74de0 to d014eed Compare October 2, 2021 17:30

jjlin approved these changes Oct 2, 2021

View reviewed changes

dani-garcia merged commit 395979e into dani-garcia:main Oct 8, 2021

domdomegg deleted the domdomegg/single-organization-policy branch October 9, 2021 00:25

domdomegg mentioned this pull request Oct 9, 2021

2FA org policy: do not enforce on invited (not accepted) users #2033

Merged

domdomegg added a commit to domdomegg/bw_web_builds that referenced this pull request Oct 9, 2021

Stop hiding single organization policy

976a702

The single organization is now supported by vaultwarden, as per dani-garcia/vaultwarden#1991 We can therefore stop hiding the setting in the web UI.

domdomegg mentioned this pull request Oct 9, 2021

Stop hiding single organization policy dani-garcia/bw_web_builds#50

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Support single organization policy #1991

feature: Support single organization policy #1991

domdomegg commented Sep 24, 2021

williamdes Sep 25, 2021

jjlin left a comment

jjlin Sep 26, 2021

domdomegg Sep 27, 2021

jjlin Sep 26, 2021

domdomegg Sep 27, 2021

jjlin Sep 26, 2021

domdomegg Sep 27, 2021

jjlin Sep 26, 2021

domdomegg Sep 27, 2021

jjlin Sep 30, 2021

jjlin left a comment

jjlin Sep 30, 2021

domdomegg Oct 2, 2021

jjlin Oct 2, 2021

jjlin Sep 30, 2021

domdomegg Oct 2, 2021

jjlin Sep 30, 2021

domdomegg Oct 2, 2021

domdomegg commented Oct 9, 2021

feature: Support single organization policy #1991

feature: Support single organization policy #1991

Conversation

domdomegg commented Sep 24, 2021

Choose a reason for hiding this comment

jjlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domdomegg commented Oct 9, 2021