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-577] - ACCESS POLICY fixing issue with user being able to update a secret if they are assi… #2763

Merged
merged 11 commits into from
Mar 7, 2023
Merged

[SM-577] - ACCESS POLICY fixing issue with user being able to update a secret if they are assi… #2763

merged 11 commits into from
Mar 7, 2023

Conversation

cd-bitwarden
Copy link
Contributor

…gning it to a project that has read/write permissions. Even though the customer is only allowed to read.

Type of change

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

Objective

Disable users from updating a secret they aren't allowed to update

Code changes

Updated UpdateSecretCommand to check the existing secret's project permissions, not the newly assigned secrets project permissions

  • file.ext: Description of what was changed and why

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
fixing issue with user being able to update a secret if they are assi…
cdb7c73 
…gning it to a project that has read/write permissions. Even though the customer is only allowed to read.
@cd-bitwarden cd-bitwarden requested a review from a team March 2, 2023 20:34
patrick-bitwarden
patrick-bitwarden previously approved these changes Mar 6, 2023
View reviewed changes
Copy link
Member

@patrick-bitwarden patrick-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

@cd-bitwarden cd-bitwarden changed the title fixing issue with user being able to update a secret if they are assi… [SM-577] - ACCESS POLICY fixing issue with user being able to update a secret if they are assi… Mar 6, 2023
Hinton
Hinton reviewed Mar 7, 2023
View reviewed changes
Copy link
Member

@Hinton Hinton left a comment

Choose a reason for hiding this comment

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

Minor nitpick with the naming looks good otherwise.

bitwarden_license/src/Commercial.Core/SecretsManager/Commands/Secrets/UpdateSecretCommand.cs Outdated Show resolved Hide resolved
Hinton
Hinton approved these changes Mar 7, 2023
View reviewed changes
Copy link
Member

@Hinton Hinton left a comment

Choose a reason for hiding this comment

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

LGTM!

@cd-bitwarden cd-bitwarden merged commit 7334de6 into master Mar 7, 2023
@cd-bitwarden cd-bitwarden deleted the SM-577-AP branch March 7, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hinton Hinton Hinton approved these changes

@patrick-bitwarden patrick-bitwarden Awaiting requested review from patrick-bitwarden

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

Successfully merging this pull request may close these issues.
3 participants
@cd-bitwarden @Hinton @patrick-bitwarden