Authorization deep dive amendments

[eliykat]

🎟️ Tracking

N/A

📔 Objective

Some follow-up changes to the authorization deep dive based on some recent discussions and experience.

The main change is to state that AuthorizationService should be called from the controller, rather than in the core layer (the query/command class). There are several reasons for this:

1. I still think that authz is fundamentally an api layer concern, mixing it in with business logic in the core layer makes everything more complex. It's useful to think of these separately.
2. Putting it in the command/query limits their re-use. Some queries may be used internally to check state, and are not returned out to the user. Some commands are used as side-effects with fixed argument values that need to be executed for all users, or may be executed by a job that is not tied to a user. Baking authorization into the main command/query logic really confuses things here.
3. It's awkward to inject the ClaimsPrincipal into the core layer. On the controller it's always available as User.
4. It's more obvious and easier to audit if there's a call in the controller before we enter the core layer.
5. We've already done this and it's been working well.

I have additional ideas around how to handle commands with complex authorization requirements, however I'd rather this guide follow our practice rather than vice versa. I don't want to overspecify here and keep having to amend it. We'll continue to iterate on it.

Other minor changes:

1. Refer to the Bit.Core.Exceptions.NotFoundException rather than NotFoundError (typo)
2. Don't refer to AuthorizeOrThrowAsync. I never added this overload and in retrospect I think it obscures things.

⏰ 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

[github-actions]

Checkmarx One – Scan Summary & Details – b38be27b-18f2-4136-8489-5de7846420eb

No New Or Fixed Issues Found

[cloudflare-workers-and-pages]

Deploying contributing-docs with    Cloudflare Pages

Latest commit:	59df5b4
Status:	✅  Deploy successful!
Preview URL:	https://b5e1b8db.contributing-docs.pages.dev
Branch Preview URL:	https://authorization-tweaks.contributing-docs.pages.dev

View logs

[withinfocus]

From a guidance perspective I know we have gone back and forth on this one, so is the right answer that it's case-by-case? @mzieniukbw has bitwarden/server#4741 which is pretty clean. Is the callout then to shift the logic when command-chaining comes up? I mostly understand your numbered points but it focuses on being API-centric and that might not be the case for us.

Separately, we should discuss AuthorizeOrThrowAsync -- can you elaborate what you think it's obscuring? Is it simply the exception? For something so heavily used I think not repeating the success check is a positive.

[eliykat]

The main question here is how tightly coupled authorization and C/Qs should be.

In very simple situations, they can be totally decoupled. e.g. See SecretsController.CreateAsync. It's the endpoint for creating a secret, you call AuthorizationService with the secret and SecretOperations.Create. Pretty straightforward. SM have implemented their entire app this way and we've started to do this in AC Team as well. It means the C/Q can focus on business logic.

In more complex situations, the command does multiple things, all of which need to be authorized, and that may not be clear from the controller. In this case I'm going to explore an interface where the command can return a list of its authorization requirements, without making the authz call itself. This is a loose coupling where the controller can still handle authorization without having to know the inner workings of the command; and the command doesn't need to know how authorization works. I want to try it before documenting it but I think this is a better level of abstraction. (It was Gibson's suggestion initially, I can't take too much credit.)

The current guide has the tightest coupling, where the C/Q must call AuthorizationService directly. I suggested it to try to simplify things but I think it's undesirable for the reasons in my first post.

I would prefer to have a clear rule about where we do authorization checks, so that they are less likely to be overlooked or missed. (e.g. assuming it's handled by the C/Q class when it's not.) However if it's something that we need to work over more in practice before documenting here then we can do that. I'm a bit wary about over-documenting or documenting prematurely if individual teams want to experiment.

---

AuthorizeOrThrowAsync I feel less strongly about; I can implement it if you think it would be useful.

[eliykat]

After considering our approach further, I have:

1. restored references to AuthorizeOrThrowAsync and submitted a PR to add it to the code: Add AuthorizeOrThrowAsync extension method server#4790
2. weakened the guidance on where authorization checks should occur so that teams can experiment with what works best for their domain.

[withinfocus]

Solid middle ground I'd say.