[AC-1387] Resource-based authentication for Groups

[eliykat]

Type of change

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

Objective

This is a proposal for us to adopt Resource-based authentication to manage our permission checks on the server.

Outline

The basic outline of this is:

• we have resources, which in our case includes ciphers, groups, organizations, etc
• we define operations, such as create/read/update/delete
• we define authentication handlers which contain the business logic deciding whether a user can perform an operation on a resource

Controllers can then call _authorizationService.AuthorizeAsync(user, resource, operation) and get back a success or failure result.

Implementation notes

I've implemented this on the Groups controller as a proof of concept.

A few further notes on choices I made here (opinionated):

• operations should be granular and specific to the resource. For example, I've made "update group", "add members to group", and "remove members from group" all separate operations, even if they share the same code in the handler. This makes it easy to use the correct operation when calling authorizationService.
• authentication handlers should replace the role and permission logic in CurrentContext. In particular, I think the getters on CurrentContext can obscure what permissions are really being granted to whom. In this model, CurrentContext is mostly used to construct objects from claims and make them available for auth handlers.
• I've added a proper exception we can throw where resource authorization fails, instead of incorrectly using NotFoundException (unless there's a reason we use this?)

Benefits

I think this is an improvement because:

1. authorization logic is currently spread between controller, services, and currentContext getters. CurrentContext exposes a mixture of permissions (ManageGroups) and roles (OrganizationAdmin). This is difficult to keep track of and change. Authorization handlers provide a central location to manage permissions for each resource.
2. our authorization logic is quickly becoming more complex with upcoming initiatives and needs to be better encapsulated
3. CurrentContext has too many duties and has too much indirection in it (getters are several layers deep). Auth handlers can be tightly scoped to their resource and particular operations (not just "manage"), making them easy to understand and less risky to change
4. almost everything can be thought of as a resource: organizations, orgUsers, providers, ciphers, groups, collections, connections, etc. We can apply it most places
5. this is an ASP.NET-native pattern - we're not reinventing the wheel

Drawbacks

1. Some resources manage access in their database queries (e.g. getting all ciphers a user has access to) - so the pattern isn't totally universal. I'm OK with this, just something worth noting
2. In this implementation, bulk operations such as "read all groups" use the organization handler, not the group handler, because we can't specify any single group object that we want to access. In other words: the resource is the org, not a group, which is unintuitive.
3. ??? your objection here. I haven't used this before, so there might be some "gotchas" I'm not thinking of.

Code changes

• src/Core/OrganizationFeatures/AuthorizationHandlers/GroupOperations.cs - defines the operations we can perform on groups
• src/Core/OrganizationFeatures/AuthorizationHandlers/GroupAuthHandler.cs - auth handler containing the business logic for performing operations on groups. This is basically the CurrentContext.ManageGroups logic.
• same for organization operation & handler
• add ResourceAuthorizationFailedException
• add getter on CurrentContext so we can access organizations constructed from claims

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

[jlf0dev]

Thank you for doing this! Permissions are something I've wanted to clean up for a while considering how difficult they are to work with.

I'm in favor of your idea behind having resources and operations for permission checks. We have a problem with intermingling user role code and custom permissions that lead to missed edge cases and bugs.

Do you have an idea yet of all the mappings between actions and current custom permissions? I would take that to design for feedback before starting, considering how many cases you'll have to account for. There are also areas we don't have permissions for yet, such as billing, that are mainly role based (which you could still create an auth handler for, just checking roles behind the scenes).

Another interesting point to consider is that there are multiple relationships to edit on a domain object such as a Collection. You can edit the Users, you can edit the associated Groups, or you can edit the domain object itself. I don't know if we want to break it down into this level of granularity, but without it there are questions we have to answer for the clients. For instance, if you can "Manage Groups" but not "Manage Collections", can you change the associated Collections on a Group? Does "Manage Groups" implicitly give you the "View All Collections" permission? Today it does.

I don't really have any opinions about the use of the built in authorization service, but seems to be a good idea to me!

Also, you can check out Danielle's RBAC work for the Admin Portal here if you're interested.

[justindbaur]

I totally love this. It puts all this business logic into Requirements and their specified handlers so it makes it really easy to make them into policies that we can apply with an [Authorize] filter if it's simple enough. You specific ones have to do with more advanced scenarios though so policies are complex enough.

I really like where this is headed.

[MGibson1]

Nice work! Let my 2 cents as well, but I think this is a clear step up.

[eliykat]

@jlf0dev

To answer your second question first:

Another interesting point to consider is that there are multiple relationships to edit on a domain object such as a Collection. You can edit the Users, you can edit the associated Groups, or you can edit the domain object itself. I don't know if we want to break it down into this level of granularity, but without it there are questions we have to answer for the clients. For instance, if you can "Manage Groups" but not "Manage Collections", can you change the associated Collections on a Group? Does "Manage Groups" implicitly give you the "View All Collections" permission? Today it does.

I think there are 2 questions in here:

1. What is the resource when we want to change the relationships between two objects? Is associating a User with a Group in the User auth handler or the Group auth handler? I think @justindbaur provided the answer in his comment above: the resource is the GroupUser, and it has its own auth handler. Same as OrganizationUser, which we already think of in this way. And yes, I would definitely be this granular.

2. How do Custom Permissions work, and what operation does each Custom Permission allow? I don't know, but we don't need to answer that question now. We can refactor to implement this pattern while maintaining all existing behaviour. e.g. In this PR, I've just moved the existing permission checks into auth handlers, and flattened out any calls to the CurrentContext getters. This doesn't change any business logic, but it's easier to see who can do what, so we can revisit this question later and tighten up any inconsistencies.

Do you have an idea yet of all the mappings between actions and current custom permissions? I would take that to design for feedback before starting, considering how many cases you'll have to account for. There are also areas we don't have permissions for yet, such as billing, that are mainly role based (which you could still create an auth handler for, just checking roles behind the scenes).

As above - I'd just maintain current behaviour. Where there are role based checks only, I'd come up with sensible operation names for them (e.g. view subscription, edit subscription, edit payment method, view billing history) and then move the role based check into the auth handler. It's not too different to what I've done here - "ManageGroups" is very broad, I've broken it up into granular operations and then moved that check to the handler.

[differsthecat]

This looks great! My .Net is a bit rusty, but we did role-based authentication at a previous company and there are a decent amount of similarities here. I only have one question on a potential improvement!

[justindbaur]

So much string Guid replacement and I love it.

[eliykat]

Thanks for the review @justindbaur, all your feedback should be addressed.

[justindbaur]

Looks good, very nice work!

[eliykat]

Mismatch between resource and operation type

[shane-melton]

Permissions only exists when org.Type == OrganizationUserType.Custom otherwise its null and will throw an exception here. (I just ran into this myself in the collection access handler)

Probably easiest to just group all of the org.Permissions.* checks and && them with a null check when there are this many permissions to check.

[eliykat]

I instantiated a default Permissions object on CurrentContentOrganization so it'll never be null :) no null checks required!

[bitwarden-bot]

Checkmarx One – Scan Summary & Details – a2c94b3b-edcf-4d17-8a2c-971dc482d5a2

New Issues

Severity	Issue	Source File / Package	Checkmarx Insight
	CSRF	/src/Api/Controllers/GroupsController.cs: 152	Attack Vector
	CSRF	/src/Api/Controllers/GroupsController.cs: 123	Attack Vector
	CSRF	/src/Api/Controllers/GroupsController.cs: 97	Attack Vector
	CSRF	/src/Api/Controllers/GroupsController.cs: 84	Attack Vector

Fixed Issues

Severity	Issue	Source File / Package	Checkmarx Insight
	CSRF	/src/Api/Controllers/GroupsController.cs: 176	Attack Vector
	CSRF	/src/Api/Controllers/GroupsController.cs: 146	Attack Vector
	CSRF	/src/Api/Controllers/GroupsController.cs: 118	Attack Vector
	CSRF	/src/Api/Controllers/GroupsController.cs: 101	Attack Vector
	CSRF	/src/Api/Controllers/GroupsController.cs: 68	Attack Vector

[eliykat]

We implemented this pattern for collections as part of Flexible Collections. We have a spike to review that as a team before rolling the pattern out to other resources. In the meantime, this PR served its purpose as a proof of concept and will be closed.