[PM-12479] - Adding group-details endpoint

[jrmccannon]

🎟️ Tracking

PM-12479

📔 Objective

This is step one in restricting the data provided by organizations/{id}/groups. If the feature flag is enabled, it will return only the group value with the collections null. In the future, the collections property will be removed. This also includes a new endpoint organizations/{id}/group-details which will return the group and collections only for admins, owners, providers and users allowed to manage groups or members.

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

[eliykat]

This class needs to be updated a little to follow the newest patterns.

1. move it out of Vault ownership - this was a mistake when it was first merged. I suggest Core/AdminConsole/OrganizationFeatures/Groups/Authorization.
2. see the OrganizationUserUserMiniDetailsAuthorizationHandler as an example for how to structure handlers. Note in particular:
   • use of OrganizationScope as the resource, rather than putting the orgId in the requirement (this represents "read all resources of X type for org Y" rather than particular resource IDs)
   • calling context.Succeed from the public method - relatively minor but cleans up the implementation a little
   • I would've said separate handlers and OperationRequirement classes for separate resource types (e.g. GroupAuthorizationHandler vs GroupDetailsAuthorizationHandler) but that causes a lot of overhead - so I think what you've done here is OK. Just make the operations very explicit, e.g. ReadAllGroups vs. ReadAllGroupDetails.

We're still refining our implementation of these so I'm not sure how clear this all is. See the Authentication Deep Dive for more info or feel free to send me a meeting invite if you want to sync on it.

[eliykat]

Why remove groups from the top-level route here?

[jrmccannon]

The top level was removed since the new route would be organizations/{id}/group-details since group details isn't following /groups. I believe the only way to override that would be to put a / at the start of the route, but then you'd override any additional route pathing that was set up higher in the stack.

[eliykat]

In that case I suggest leaving groups in the top-level route and using details at the method level, i.e. organizations/{id}/groups/details. (We already do this for collection details.) My main concern is that all our controllers use a controller-level route, this would be an outlier that is easily missed. This also keeps all groups-related resources under the groups path which I think might be better REST design (although that might be arguable).

[eliykat]

I see I specified group-details in the Jira ticket, that's my bad!

[jrmccannon]

No worries. Easy fix. I'll move group back to the top and make this one details. I'll update the client code accordingly as well.

[eliykat]

This is optional depending on how deep you want to go with testing. But xUnit supports an attribute called MemberData which can be used to parameterize tests with complex objects: https://hamidmosalla.com/2017/02/25/xunit-theory-working-with-inlinedata-memberdata-classdata/ (scroll down to the Member Data heading). All these tests are quite similar, I wonder if you could combine almost all of them by defining an object containing data for all test cases (e.g. all combinations of role, custom permissions, expectedResult) and consuming it with the MemberData attribute in a single test.

We have a wrapper around it called BitMemberAutoDataAttribute which may provide some examples.

[codecov]

Codecov Report

Attention: Patch coverage is 55.81395% with 19 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@6cc097e). Learn more about missing BASE report.
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...c/Api/AdminConsole/Controllers/GroupsController.cs	0.00%	16 Missing ⚠️
.../Groups/Authorization/GroupAuthorizationHandler.cs	85.00%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4959   +/-   ##
=======================================
  Coverage        ?   42.55%           
=======================================
  Files           ?     1387           
  Lines           ?    64642           
  Branches        ?     5934           
=======================================
  Hits            ?    27507           
  Misses          ?    35911           
  Partials        ?     1224           

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