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.

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

Delete duplicate BulkCollectionOperation.ReadWithAccess #4029

Conversation

eliykat
Copy link
Member

@eliykat eliykat commented Apr 29, 2024

Type of change

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

Objective

A number of us were working on the BulkCollectionAuthorizationHandler at the same time, and we ended up with some duplicate BulkCollectionOperations:

  • Read
  • ReadAccess
  • ReadWithAccess

Read and ReadAccess both shared the same private method in the handler; ReadWithAccess had its own private method which was identical, except that it also authorized the Manage Users custom permission.

We should favour more granular operations, so that "read with access" should be Read && ReadAccess, without the need for the third operation. This also reduces duplicate code.

Code changes

  1. delete BulkCollectionOperation.ReadWithAccess, its private method in the handler, and its tests
  2. update the usage in the controller to check Read && ReadAccess instead
  3. add the Manage Users custom permission to the existing BulkCollectionAuthorizationHandler.CanReadAsync private method, which is used by Read and ReadAccess operations. This ensures no change to the actual authorization logic.

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

@eliykat eliykat requested review from a team as code owners April 29, 2024 04:57
@eliykat eliykat requested review from LRNcardozoWDF, addisonbeck and r-tome and removed request for addisonbeck April 29, 2024 04:57
Copy link

codecov bot commented Apr 29, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 38.11%. Comparing base (ba36b2d) to head (a80962b).

Files Patch % Lines
src/Api/Controllers/CollectionsController.cs 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4029      +/-   ##
==========================================
- Coverage   38.13%   38.11%   -0.03%     
==========================================
  Files        1195     1195              
  Lines       58194    58172      -22     
  Branches     5576     5571       -5     
==========================================
- Hits        22192    22171      -21     
  Misses      34956    34956              
+ Partials     1046     1045       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

github-actions bot commented Apr 29, 2024

Logo
Checkmarx One – Scan Summary & Details91fcd390-72e9-408b-b91f-3707ee8ff672

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM CSRF /src/Api/Billing/Public/Controllers/OrganizationController.cs: 44 Attack Vector
MEDIUM CSRF /src/Api/Controllers/SelfHosted/SelfHostedOrganizationLicensesController.cs: 71 Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 376 Attack Vector
MEDIUM CSRF /src/Api/Controllers/SelfHosted/SelfHostedOrganizationLicensesController.cs: 97 Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 526 Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 303 Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 512 Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 794 Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 852 Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 244 Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 827 Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationsController.cs: 407 Attack Vector

@eliykat eliykat marked this pull request as draft April 30, 2024 06:16
@eliykat
Copy link
Member Author

eliykat commented Apr 30, 2024

Converting to draft until #4032 is merged as that will cause conflicts.

Copy link
Contributor

@r-tome r-tome left a comment

Choose a reason for hiding this comment

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

LGTM!

@eliykat
Copy link
Member Author

eliykat commented Jul 12, 2024

This is out of date and I'm not particularly satisfied with it. I'm going to revisit this as part of authorization generally.

@eliykat eliykat closed this Jul 12, 2024
@eliykat eliykat deleted the ac/ac-2326/delete-duplicate-bulkcollection-operation-readwithaccess-or-readaccess branch July 12, 2024 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants