[PM-14826] Add UsePolicies check to GET endpoints #5046
Conversation
GetByToken and GetMasterPasswordPolicy endpoints provide policy information, so if the organization is not using policies, then we avoid the rest of the logic.
| } | ||
|
|
||
| [HttpGet("{type}")] | ||
| public async Task<PolicyDetailResponseModel> Get(Guid orgId, int type) | ||
| { | ||
| if (!await _currentContext.ManagePolicies(orgId)) |
There was a problem hiding this comment.
A lot of these were auto-formatted. Please let me know if it's too much noise, and I'll remove them.
There was a problem hiding this comment.
Reordering the private properties is fine, but please re-add all the curly brackets to keep the code consistent with the rest of the codebase.
|
|
||
| // Act & Assert | ||
| await Assert.ThrowsAsync<NotFoundException>(() => | ||
| sutProvider.Sut.GetByToken(orgId, email, token, organizationUserId)); |
There was a problem hiding this comment.
Question: This method, GetByToken, hasn't had any unit tests before. Should I add new unit tests for cases outside my scope? I'm happy to do so, just want to check in on the expectation.
There was a problem hiding this comment.
Feel free to add more unit tests if you'd like! While it's not expected for work outside the ticket scope, I'm a big advocate for maximizing test coverage. I really appreciate seeing that effort, kudos!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5046 +/- ##
=======================================
Coverage 43.05% 43.05%
=======================================
Files 1409 1409
Lines 64710 64688 -22
Branches 5915 5917 +2
=======================================
- Hits 27859 27854 -5
+ Misses 35622 35602 -20
- Partials 1229 1232 +3 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
| } | ||
|
|
||
| [HttpGet("{type}")] | ||
| public async Task<PolicyDetailResponseModel> Get(Guid orgId, int type) | ||
| { | ||
| if (!await _currentContext.ManagePolicies(orgId)) |
There was a problem hiding this comment.
Reordering the private properties is fine, but please re-add all the curly brackets to keep the code consistent with the rest of the codebase.
|
|
||
| // Act & Assert | ||
| await Assert.ThrowsAsync<NotFoundException>(() => | ||
| sutProvider.Sut.GetByToken(orgId, email, token, organizationUserId)); |
There was a problem hiding this comment.
Feel free to add more unit tests if you'd like! While it's not expected for work outside the ticket scope, I'm a big advocate for maximizing test coverage. I really appreciate seeing that effort, kudos!
|
No New Or Fixed Issues Found |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-14826
📔 Objective
GET organizations/{{orgId}}/policies/master-passwordandGET organizations/{{orgId}}/policies/token📸 Screenshots
manually tested the
GET organizations/{{orgId}}/policies/tokenFor a free Org (sad path).
For an enterprise Org (happy path).
Note: I'm still tracking down the
GET organizations/{{orgId}}/policies/master-passwordin the UI. I will circle back here when I find it.⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes