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

Only org policy #962

Merged
merged 23 commits into from
Oct 20, 2020
Merged

Only org policy #962

merged 23 commits into from
Oct 20, 2020

Conversation

addisonbeck
Copy link
Contributor

@addisonbeck addisonbeck commented Oct 8, 2020

Narrative ✍🏻

As an enterprise organization admin I want to be able to block members of my organization from being able to be a part of other organizations.

Acceptance Criteria Relevant To This PR 💯

  1. A policy (OnlyOrg) exists that restricts non-admin members from being able to be a part of other organizations.
  2. When applying the OnlyOrg policy any non-owner members who aren't the one enabling the policy & that are already in another org should be removed from the org applying the policy. This user flow should mirror what already exists for the 2FA policy.
  3. Removed users should get an email informing them of the removal.
  4. The OnlyOrg policy needs to be configurable from the web vault AND the enterprise portal.
  5. Users in an org with the OnlyOrg policy enabled should not be able to create new organizations.

Code Changes 👨🏻‍💻

API

  • Added a new PolicyType enum
  • Blocked accepting new organization invites when OnlyOrg is relevant to the user
  • Blocked creating new orgs if already in an org with OnlyOrg enabled (this is also checked on the client)
  • Blocked confirming any org members that may have violated OnlyOrg between accepting their invite and being confirmed
  • Created an email alert to be sent when a user is kicked because of the OnlyOrg policy
  • Removed users and sent alerts when appropriate (this involved a slight refactor of the same logic for 2FA)

Portal

  • Added any needed localization strings for policy config in the portal
  • Allowed for the OnlyOrg policy to be configured on the policies screen

Screenshots

Screen Shot 2020-10-08 at 1 29 23 PM

Screen Shot 2020-10-08 at 1 29 29 PM

Notes 📝

Asana ticket
Web PR
jslib PR

Comment on lines 84 to 85
case PolicyType.OnlyOrg:
break;
Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't the 2FA policy enter this same code block? I would think the onlyorg and 2fa policies would be set up the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 2FA policy wasn't represented in this switch at all, which was causing a bug: enabling the 2FA policy from the portal simply didn't work.

I noticed this, but it didn't seem in scope of this PR so I was planning to fast-follow and fix it later, like with deleting orgUser records with an org, but since you brought it up I went ahead and fixed it here 😁

src/Api/Controllers/OrganizationsController.cs Outdated Show resolved Hide resolved
src/Api/Controllers/OrganizationsController.cs Outdated Show resolved Hide resolved
src/Core/Services/Implementations/OrganizationService.cs Outdated Show resolved Hide resolved
src/Core/Services/Implementations/OrganizationService.cs Outdated Show resolved Hide resolved
src/Core/Services/Implementations/OrganizationService.cs Outdated Show resolved Hide resolved
src/Core/Services/Implementations/PolicyService.cs Outdated Show resolved Hide resolved
if (orgUser.UserId.HasValue)
{
var userOrgs = await _organizationUserRepository.GetManyByUserAsync(orgUser.UserId.Value);
if (userOrgs.Count > 1)
Copy link
Member

Choose a reason for hiding this comment

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

same. i think you need to check status

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checked

src/Core/Services/Implementations/PolicyService.cs Outdated Show resolved Hide resolved
bitwarden_license/src/Portal/Models/PolicyEditModel.cs Outdated Show resolved Hide resolved
src/Api/Controllers/OrganizationsController.cs Outdated Show resolved Hide resolved
src/Api/Controllers/OrganizationsController.cs Outdated Show resolved Hide resolved
src/Api/Controllers/OrganizationsController.cs Outdated Show resolved Hide resolved
src/Core/Enums/PolicyType.cs Outdated Show resolved Hide resolved
src/Core/Services/Implementations/OrganizationService.cs Outdated Show resolved Hide resolved
src/Core/Services/Implementations/OrganizationService.cs Outdated Show resolved Hide resolved
src/Core/Services/Implementations/OrganizationService.cs Outdated Show resolved Hide resolved
src/Core/Services/Implementations/PolicyService.cs Outdated Show resolved Hide resolved
@addisonbeck
Copy link
Contributor Author

@cscharf @kspearrin this one got a little muddy with the string changes (I'll try and run those by you guys before PR next time), but I think the only issue left open is this one regarding optimizing removal of users on policy enable. Let me know if you'd like me to make the change I suggested on that thread, or any others.

Copy link
Contributor

@cscharf cscharf left a comment

Choose a reason for hiding this comment

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

Will still need @kspearrin to review from his requested changes and approval before this can be merged.

throw new BadRequestException("You are already part of this organization.");
}

if (!CoreHelpers.UserInviteTokenIsValid(_dataProtector, token, user.Email, orgUser.Id, _globalSettings))
Copy link
Member

Choose a reason for hiding this comment

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

Where did this token validation go?

Copy link
Contributor Author

@addisonbeck addisonbeck Oct 19, 2020

Choose a reason for hiding this comment

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

Was this comment looking at a specific commit? This logic was moved around in this PR, it's reflected in some commits here because of merge issues.

@addisonbeck addisonbeck merged commit e872b4d into master Oct 20, 2020
@addisonbeck addisonbeck deleted the OnlyOrgPolicy branch October 20, 2020 06:48
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.

3 participants