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.

Sign up for GitHub

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

Jump to bottom

[SM-896] restricting access to disabled orgs #3287

Merged
merged 13 commits into from
Oct 16, 2023
Merged

[SM-896] restricting access to disabled orgs #3287

merged 13 commits into from
Oct 16, 2023

Conversation

cd-bitwarden
Copy link
Contributor

Type of change

- [ ] Bug fix
- [ x ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Disabled access to api endpoints that are not longer enabled

Code changes

src/Core/Context/CurrentContextOrganization.cs - Added check for orgUser.Enabled
src/Identity/IdentityServer/ClientStore.cs - Added a check for org.Enabled

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • If making database changes - make sure you also update Entity Framework queries and/or migrations
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

@cd-bitwarden cd-bitwarden requested a review from a team as a code owner September 21, 2023 13:46
@bitwarden-bot
Copy link

bitwarden-bot commented Sep 21, 2023 •
edited
Loading

Logo
Checkmarx One – Scan Summary & Details – 45a11e7b-e103-4730-86d0-98b2d90271e7

No New Or Fixed Issues Found

Thomas-Avery
Thomas-Avery requested changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@Thomas-Avery Thomas-Avery left a comment

Choose a reason for hiding this comment

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

Looks good!

We should update all the Secrets Manager endpoint SmNotEnabled integration tests to test for this new condition.

Here is an example of one

server/test/Api.IntegrationTest/SecretsManager/Controllers/SecretsControllerTests.cs

Lines 58 to 69 in 90d600d

[Theory]
[InlineData(false, false)]
[InlineData(true, false)]
[InlineData(false, true)]
public async Task ListByOrganization_SmNotEnabled_NotFound(bool useSecrets, bool accessSecrets)
{
var (org, _) = await _organizationHelper.Initialize(useSecrets, accessSecrets);
await LoginAsync(_email);
var response = await _client.GetAsync($"/organizations/{org.Id}/secrets");
Assert.Equal(HttpStatusCode.NotFound, response.StatusCode);
}

@cd-bitwarden cd-bitwarden changed the title restricting access to disabled orgs [SM-896] restricting access to disabled orgs Sep 21, 2023
@cd-bitwarden cd-bitwarden requested a review from a team as a code owner September 21, 2023 20:49
Thomas-Avery
Thomas-Avery previously approved these changes Sep 25, 2023
View reviewed changes
Copy link
Contributor

@Thomas-Avery Thomas-Avery left a comment

Choose a reason for hiding this comment

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

Changes look good from SM side.

We will want auth to review as well.

JaredSnider-Bitwarden
JaredSnider-Bitwarden reviewed Oct 9, 2023
View reviewed changes
Copy link
Contributor

@JaredSnider-Bitwarden JaredSnider-Bitwarden left a comment

Choose a reason for hiding this comment

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

This is looking great! Only two minor questions.

test/Api.IntegrationTest/SecretsManager/Controllers/AccessPoliciesControllerTests.cs Show resolved Hide resolved
test/Api.IntegrationTest/SecretsManager/SecretsManagerOrganizationHelper.cs Outdated Show resolved Hide resolved
@cd-bitwarden cd-bitwarden requested a review from a team as a code owner October 9, 2023 17:45
JaredSnider-Bitwarden
JaredSnider-Bitwarden approved these changes Oct 9, 2023
View reviewed changes
Copy link
Contributor

@JaredSnider-Bitwarden JaredSnider-Bitwarden left a comment

Choose a reason for hiding this comment

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

LGTM!

Thomas-Avery
Thomas-Avery approved these changes Oct 9, 2023
View reviewed changes
Copy link
Contributor

@Thomas-Avery Thomas-Avery left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@cd-bitwarden cd-bitwarden enabled auto-merge (squash) October 16, 2023 14:28
@cd-bitwarden cd-bitwarden merged commit b772784 into master Oct 16, 2023
37 of 38 checks passed
@cd-bitwarden cd-bitwarden deleted the SM-896 branch October 16, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JaredSnider-Bitwarden JaredSnider-Bitwarden JaredSnider-Bitwarden approved these changes

@Thomas-Avery Thomas-Avery Thomas-Avery approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cd-bitwarden @bitwarden-bot @Thomas-Avery @JaredSnider-Bitwarden