Merge branch 'mk/add-guidelines-for-where-to-do-permission-checks' into 'master'

Add guidelines for where to check permissions

See merge request gitlab-org/gitlab!55142

doc/development/permissions.md

@@ -120,3 +120,31 @@ into different features like Merge Requests and CI flow.
 | View | Vulnerability feedback | Merge Request | Can read security findings |
 | View | Dependency List page | Project | Can access Dependency information |
 | View | License Compliance page | Project | Can access License information|
+
+## Where should permissions be checked?
+
+By default, controllers, API endpoints, and GraphQL types/fields are responsible for authorization. See [Secure Coding Guidelines > Permissions](secure_coding_guidelines.md#permissions).
+
+### Considerations
+
+- Many actions are completely or partially extracted to services, finders, and other classes, so it is normal to do permission checks "downstream".
+- Often, authorization logic must be incorporated in DB queries to filter records.
+- `DeclarativePolicy` rules are relatively performant, but conditions may perform database calls.
+- Multiple permission checks across layers can be difficult to reason about, which is its own security risk. For example, duplicate authorization logic could diverge.
+- Should we apply defense-in-depth with permission checks? [Join the discussion](https://gitlab.com/gitlab-org/gitlab/-/issues/324135)
+
+### Tips
+
+If a class accepts `current_user`, then it may be responsible for authorization.
+
+### Example: Adding a new API endpoint
+
+By default, we authorize at the endpoint. Checking an existing ability may make sense; if not, then we probably need to add one.
+
+As an aside, most endpoints can be cleanly categorized as a CRUD (create, read, update, destroy) action on a resource. The services and abilities follow suit, which is why many are named like `Projects::CreateService` or `:read_project`.
+
+Say, for example, we extract the whole endpoint into a service. The `can?` check will now be in the service. Say the service reuses an existing finder, which we are modifying for our purposes. Should we make the finder check an ability?
+
+- If the finder doesn't accept `current_user`, and therefore doesn't check permissions, then probably no.
+- If the finder accepts `current_user`, and doesn't check permissions, then it would be a good idea to double check other usages of the finder, and we might consider adding authorization.
+- If the finder accepts `current_user`, and already checks permissions, then either we need to add our case, or the existing checks are appropriate.