Skip to content

[EC-235] Give Admins (and above) access to all items #2036

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
Merged
merged 3 commits into from
Jun 7, 2022
Merged

[EC-235] Give Admins (and above) access to all items #2036

merged 3 commits into from
Jun 7, 2022

Conversation

@eliykat
Copy link
Member

@eliykat eliykat commented Jun 6, 2022

Type of change (mark with an X)

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

Objective

Fix an issue where admins cannot see collections that they create.

This is the same issue as #1959, I just didn't cast the net wide enough. Instead of just checking for Owners (and implicitly Providers), we should also check for Admins - as they also have access to all items.

Code changes

  • change _currentContext.organizationOwner to _currentContext.organizationAdmin (which implicitly includes owners and providers as well)
  • make comment more succinct

Before you submit (mark with an X)

- [x] I have checked for formatting errors (`dotnet tool run dotnet-format --check`) (required)
- [ ] If making database changes - I have also updated Entity Framework queries and/or migrations
- [ ] I have added **unit tests** where it makes sense to do so (encouraged but not required)
- [ ] This change requires a **documentation update** (notify the documentation team)
- [ ] This change has particular **deployment requirements** (notify the DevOps team)

@eliykat eliykat requested a review from a team June 6, 2022 06:12
gbubemismith
gbubemismith reviewed Jun 6, 2022
View reviewed changes
Copy link
Contributor

@gbubemismith gbubemismith left a comment

Choose a reason for hiding this comment

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

This looks good

@eliykat eliykat requested a review from a team June 6, 2022 20:41
@eliykat
Copy link
Member Author

eliykat commented Jun 6, 2022

@gbubemismith Github isn't letting me merge this - do you have write access to the Bitwarden repos? I've requested another review from the team.

differsthecat
differsthecat previously approved these changes Jun 6, 2022
View reviewed changes
@eliykat
Copy link
Member Author

eliykat commented Jun 7, 2022

Sorry @differsthecat , the comment was misaligned and it was going to bug me for all time if I didn't fix it now

@eliykat eliykat requested a review from differsthecat June 7, 2022 00:13
@differsthecat
Copy link
Member

differsthecat commented Jun 7, 2022 •
edited
Loading

Sorry @differsthecat , the comment was misaligned and it was going to bug me for all time if I didn't fix it now

No worries!

@eliykat eliykat merged commit f602df2 into master Jun 7, 2022
@eliykat eliykat deleted the fix/admin-permissions branch June 7, 2022 00:33
@eliykat eliykat mentioned this pull request Jun 15, 2022
Merged
eliykat added a commit that referenced this pull request Jun 20, 2022
@eliykat
[EC-235] Give Admins (and above) access to all items (#2036)
0bfcc96 
(cherry picked from commit f602df2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gbubemismith gbubemismith gbubemismith left review comments

@differsthecat differsthecat differsthecat 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

@eliykat @differsthecat @gbubemismith