-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[SM-896] restricting access to disabled orgs #3287
Conversation
No New Or Fixed Issues Found |
There was a problem hiding this 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); | |
} |
test/Api.IntegrationTest/SecretsManager/Controllers/AccessPoliciesControllerTests.cs
Outdated
Show resolved
Hide resolved
…ciesControllerTests.cs Co-authored-by: Thomas Avery <[email protected]>
There was a problem hiding this 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.
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this 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!
Type of change
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
dotnet format --verify-no-changes
) (required)