The purpose of this design is to define how to enforce permissions when F3 is used to mirror a remote forge to a local forge.

For instance, mirroring https://codeberg.org/forgejo/design to a Forgejo instance running from source can be done with:

curl -sS -H "Authorization: token $F3_LOCAL_TOKEN" --json '{"url":"https://codeberg.org", "token": "'$F3_CODEBERG_ORG_TOKEN'", "type": "forgejo", "from": "/forge/organizations/forgejo/projects/design"}' http://0.0.0.0:3000/api/v1/f3

The Forgejo F3 driver based on gof3 performs the following:

• Use the API with the F3_CODEBERG_ORG_TOKEN token to read from the https://codeberg.org/forgejo/design repository
• Create a F3 archive in a directory, as defined by F3
• Read those files and modify the Forgejo database and files (just as a series of API call would) to create or update the local copy of the design repository

Since it is not using the REST API, each operation of this internal driver must verify that the doer (i.e. the user used to create the F3_LOCAL_TOKEN token in the example above) has the required permissions (for instance the author of an issue may not be allowed to set the labels).

Implementation

Draft

Note

drafts are developed and published in the forgefriends repository

• /api/v1/f3 REST API endpoint

Proposed Design

The /api/v1/f3 endpoint creates or updates the mirror of a remote repository and is given:

• The URL of the remote forge (e.g. https://codeberg.org)
• The F3 path to mirror on the remote forge (e.g. /forge/users/myuser/projects/myproject)
• A token with enough permissions to read this F3 path and all its descendants from the remote forge

The gof3 package implements the high level mirroring logic and relies on:

• the drivers provided by the gof3 package to read the remote forge. The permissions are enforced by the remote forge and is not in scope for this discussion.
• the F3 internal driver to write to the local Forgejo instance.

This driver provides the following functions for each resource:

• List (e.g. listing all comments of an issue in a way similar to GET /repos/{owner}/{repo}/issues/comments)
• Get (e.g. read a comment in a way similar to GET /repos/{owner}/{repo}/issues/comments/{id})
• Put (e.g. create a comment in a way similar to POST /repos/{owner}/{repo}/issues/{index}/comments)
• Patch (e.g. edit a comment in a way similar to PATCH /repos/{owner}/{repo}/issues/comments/{id})
• Delete (e.g. delete a comment in a way similar to DELETE /repos/{owner}/{repo}/issues/comments/{id})

Each driver function is responsible to enforce the required permissions, in the same way each Forgejo REST API endpoint is (sequence of middleware or the handler implementation).

Enforcing permissions

The Forgejo REST API permissions are implemented in permission check functions. They are called by REST API endpoints in a specific order and are tested to be effective by the tests associated with each REST API endpoint.

There is a single REST API endpoint for F3 mirroring. It is given a PAT that will be used by the F3 driver functions to perform the same sequence of of permission checks than the equivalent REST API function before performing an operation.

For instance, the Get() driver function to read a repository must perform the same permission checks as the GET /repos/{owner}/{repo} REST API endpoint.

To achieve this re-use:

• the F3 function calls the REST API permission checks in the same order they are specified in the routes of the corresponding REST API endpoint. See how the Get() driver function to read a repository calls TokenRequiresScopes,RepoAccess,CheckTokenPublicOnly and ReqAnyRepoReader in the same order the REST API route to GET /repos/{owner}/{repo} calls them as specified by the route that leads to the endpoint.

All F3 function are paired with a REST API endpoint and automated tests verify that both run the same permission checks in the same order. Figuring out which permissions checks are performed by a given REST API endpoint is not trivial. The automated tests that fail when the checks change in the REST API are essential to the maintainability of F3. It would otherwise require a scrutiny that cannot reasonably be expected from maintainers. See how the sequence of permissions checks are harvested for each REST API call when running tests and verified to match what F3 functions do by collecting calls and matching them.

Absolute ID scoping

Every F3 resource (e.g. issue, comment, ...) is guaranteed to have a parent. When the ID of the resource is absolute (e.g. comment), it can be verified to belong to the parent.

For instance, the Get function on a comment must verify the ID is related by using the issue ID obtained from the parent in the F3 tree.

It must be paired with the REST API endpoint that relies on ReqValidCommentID.

Overview of Forgejo API permissions

The permissions (aka access modes) associated with an action (REST or web) of a given doer are calculated to either be read, write, admin, owner or none.

Within a repository, permissions are automatically denied if the associated Unit is disabled (e.g. if issues are disabled on a repository). For repositories that belong to an organization, team members permissions may be further modified for Code, Issues, Pull Request, Releases. Other units are not currently relevant to F3: Project, Package, Actions, nor is Wiki. If the doer is a collaborator of a given repository, its permissions are futher modified (and supersedes team permissions) to be read, write or none.

An admin user is automatically granted the highest permissions on the instance. The anonymous user and the Ghost user are not in scope because the F3 API requires that a token is provided. The automatic token created on behalf of the Forgejo Actions user is restricted to act on the repository for which it was generated.

When an absolute ID is unrelated to its parents (e.g. a comment ID unrelated to a comment), access is denied as if it did not exist (i.e. it is 404 instead of 403).

Scopes

The token used to authentify the doer may further limit the permissions granted to the user that owns it with the following scopes:

• organization
• issue
• repository
• user

And other scopes that are not currently relevant for F3 (misc, activitypub, notifications, package).

Repository Units

The API routes have nested checks verifying the features (units) related to a resource (issue, release, etc.) are enabled. For instance mustEnableIssuesOrPull

code.forgejo.org/forgefriends/forgefriends@14075b941c/routers/api/v1/api.go (L426-L428)

forbids using any routes under /api/v1/repos/{owner}/{reponame}/issues unless the issues unit is set on a repository. Once the router checks have passed, it is the responsibility of each REST API endpoint to enforce additional restrictions

code.forgejo.org/forgefriends/forgefriends@14075b941c/routers/api/v1/repo/issue.go (L497-L508)

based on the permissions granted to the doer.

code.forgejo.org/forgefriends/forgefriends@14075b941c/models/perm/access_mode.go (L13-L24)

Absolute ID scoping

Some API routes contain identifiers that are relative to their parent (e.g. the index of an issue in /api/v1/repos/{owner}/{reponame}/issues/{index}). Others contain identifiers that are absolute (e.g. the id of a comment in /api/v1/repos/{owner}/{reponame}/issues/comments/{id}). An attempt to use an absolute ID unrelated to the resource (e.g. a repository) must be forbidden as it would escalate the permissions of the doer (e.g. read a comment from any private repository).

For instance reqValidCommentID

code.forgejo.org/forgefriends/forgefriends@14075b941c/routers/api/v1/api.go (L195-L203)

forbids using a comment ID if it does not belong to the related repository.

Doer

The doer of an REST API endpoint can either be:

• A registered user
• A system user (Ghost or Forgejo Actions)
• Anonymous (user is nil)

The Forgejo Actions automatic token is bound to the Forgejo Actions system user and subject to special restrictions on all REST API endpoints under /api/v1/repos/{owner}/{repo}.

• It is denied access (no read, no write) to repositories that are not the same as the repository where the actions is running
• It has read permissions on all units if the the action is running on a pull request and the head if it is from a forked repository
• Otherwise it has write permissions on all units in the repository

This design is ready for review. It is partially implemented in the draft repository and I will wait for input before proceeding further, in case the architecture needs to be reworked entirely.

@gusted it would be great if you can spare five minutes to read this design.If it does not trigger any remark, it is fine. If it gives you a bad feeling about anything, a terse comment will be very welcome, even if it comes with no explanation.

For the record @gusted replied via Matrix and expressed interest but is not available for a review before some time.

I will work some more on this in the near future.

@mfenniak I noticed today your work on repository and tokens and would be grateful if you could take a quick look at this draft design. At this stage it is not worth spending more than a few minutes on it. But if anything strikes you as obviously backward within this very short timeframe, I'd be glad to hear about it.

A quick background summary on F3: it is still in its early stages. In the past weeks it reached a point where I'm able to work on this issue by using a Forgejo instance that runs on my laptop and it mirrors the changes to Codeberg (this is a modified forgejo). It is on that occasion that I ran into this permission bug because it blocks mirroring the images attached to a comment. The commits of this modified Forgejo need a sound security model before they can be proposed as pull requests to Forgejo itself. It is my main motivation to get this design right.

Hey @limiting-factor, thanks for pinging me.

I think I understand the problem space at least -- there are already two modes that Forgejo does permission/access checks in (web, API), and it's probably fair to say that they share tools, but not implementations. In other words, it's quite possible for one of the two modes to have a differing implementation for any access check; a check can be missing or vary in logic easily, even though they both use common APIs like GetUserRepoPermission. If I understand the problem you're describing, the F3 system would be a bulk operation to mirror data from "over there" to "in here", and as that operates in bulk on a lot of objects "in here", it needs permission checks as well which could introduce a third mode of operating. And that has a risk that it could lack, or vary, permission checks as well.

Is that more-or-less right?

If I'm understanding, I don't feel like I have a lot of useful insight to provide that isn't obvious. So, just conceptual thoughts, and not specific implementation thoughts. 🙂

(a) It seems crucial that access checks be centralized in their logic, and duplicated less, not more.

(b) It feels like we need some form of abstraction or declarative model that allows us to broadly implement access control checks automatically, rather than imperatively on every object access. At some kind of boundary in the application (for example, when taking an API response and returning it to the user), the system could recognize that api.RepositoryMeta is a repository, and therefore the user needs to be able to view it, and therefore an access control check is performed automatically. But defining the boundaries and the checks that would allow this to happen seems really unclear to me.

I think when I look at things like code.forgejo.org/forgefriends/forgefriends@b17a3e1f6a/services/f3/permissions/comment/comment.go (L13-L25) ... I worry that it's in the direction of a third possibly access check that may deviate from two other implementations. But the problem is that I don't have a practical suggestion for how to do it better, where "practical" means "can be done today".

@mfenniak wrote:

Is that more-or-less right?

Exactly right.

(a) It seems crucial that access checks be centralized in their logic, and duplicated less, not more.

Yes. Crucial is the keyword. Each permission check has the potential of introducing a new security bug.

I worry that it's in the direction of a third possibly access check that may deviate from two other implementations.

It is.

But the problem is that I don't have a practical suggestion for how to do it better, where "practical" means "can be done today".

The more I work on this the more I'm convinced that the solution is close to what you suggested in (b). It is a lot of refactor work, with no immediate value to Forgejo (and the risk of introducing a security bug in the process). Once the web and REST API use a unified access control instead of being two different implementations, F3 could use it as well, without duplicating the logic.

I don't see how to solve that problem. I could spend all the time needed to perform this refactor (it will take months, there are so many endpoints, but it is worth it). However, even if I do that perfectly well and the pull requests I propose are flawless, there will be dozens of them, possibly hundreds and that will add an impossible workload to other Forgejo contributors. Realistically this will make progress so slowly that it will stall.

Or...

I could try to implement an access control system that is not specific to F3, although it would only be used by F3 at first. And then it could gradually be used to remove the logic duplication from the REST and web API. Every time an access control security bug needs to be fixed, it could use that common control system instead of fixing the ad-hoc access control in place for the REST or web API. And each time an API endpoint is modified or added, it would be a new opportunity to refactor the ad-hoc access control and use the common access control. It may take a very long time but I can see that making progress and coming to an end, eventually.

But defining the boundaries and the checks that would allow this to happen seems really unclear to me.

This is also unclear to me. However, even if that is not implemented, ensuring the access control logic is in one place instead of being duplicated is one step in the right direction. If, at a later time, someone figures out how to define those boundaries, they would have an easier time than with two (or three) implementations.

I refactored the draft forgefriends permissions implementation to illustrate the proposal.

• A new Forgejo service implements the permission model code.forgejo.org/forgefriends/forgefriends@adb9bd6556/services/permissions
• The storage of this permission service is supported by the implementation of an interface code.forgejo.org/forgefriends/forgefriends@adb9bd6556/services/permissions/context/interface.go (L15-L32)
  • The REST API context code.forgejo.org/forgefriends/forgefriends@adb9bd6556/services/context/api.go (L181-L223)
  • The F3 context code.forgejo.org/forgefriends/forgefriends@adb9bd6556/services/permissions/context/context.go (L48-L90)
  • The web context (to be done)
• The permission service provides functions that implement the permission logic and that can be shared between web, REST and F3, for instance:
  • routers/api/shared/middleware.go:apiAuthorization can be replaced by the function doing the same in the permissions service services/permissions/context/functions.go:SetAuthorization which is a trivial refactor using the accessors expected by the interface instead of data members of the concrete struct

@limiting-factor wrote in #61 (comment):

I could try to implement an access control system that is not specific to F3, although it would only be used by F3 at first. And then it could gradually be used to remove the logic duplication from the REST and web API. Every time an access control security bug needs to be fixed, it could use that common control system instead of fixing the ad-hoc access control in place for the REST or web API. And each time an API endpoint is modified or added, it would be a new opportunity to refactor the ad-hoc access control and use the common access control. It may take a very long time but I can see that making progress and coming to an end, eventually.

That's quite a compelling approach.

My historical experience casts skepticism over the idea. I've been a part of many development projects where "we'll introduce a new thing that solves all the problems, and then migrate piece by piece to it". I'm not sure I've ever been part of a project where the migration is actually completed. We inevitably end up with one more thing, forever. However, we don't need to let my professional failures destroy the idea 🤣... it's just fair to have a healthy skepticism, leading to figuring out how to manage risks, including the idea that it might not get completed.

It would be a reasonable statement to say that we're better off with a third set of permission checks, if it enables us to reduce to one set of permission checks over time. But, the new access control system would need to be complete enough to capture the subtle behaviours of the web backend and API backends, or else it would be destined to be the third thing forever.

While you've picked a case to implement in your draft proposal, there might be some other more complex cases that need to be proven out through this draft to sanity checking that the access control system can represent the real-world, complex, or subtle cases. (I don't have specifics in mind unfortunately.)

From just a technical coding perspective, the mutability of the PermissionsContext in your new permission model raises a red flag for me. I'm not sure if I'm understanding how it's expected to work correctly.

• code.forgejo.org/forgefriends/forgefriends@adb9bd6556/services/permissions/comment/comment.go (L20-L22)
• Calling Get(repoID, ...)(ctx) will change state on the ctx, mutating state on the ctx, impacting future permission checks, and other accesses (outside of the PermissionsContext) to the data stored on ctx.
• I don't really expect unpredictable side-effects from a Get method.
• It's quite plausible to me that a comment permission check occurs on multiple objects in one page load -- for example, viewing cross-referenced comments on an issue will require permission checks against the repo that those comments originated from -- and each of those will mutate the ctx state to different repos.

@mfenniak wrote:

That's quite a compelling approach.

I'm glad you find this an improvement over the previous (F3 only) approach.

My historical experience casts skepticism over the idea...

I'm grateful for the kind way to put it. The previous approach was guaranteed to create a code duplication problem. This approach could work but I agree that it highly depends on its adoption rate for the REST and web API. For it to succeed it needs to get over a threshold and it is up to me to get there.

... there might be some other more complex cases that need to be proven out through this draft to sanity checking that the access control system can represent the real-world, complex, or subtle cases.

Yes, and I chose comments because it is one of those non-trivial case (absolute ID check), but there are others, possibly more complicated.

There is much less variety in how mirroring is implemented in F3 compared to the REST /web API endpoints. Each resource (issue, reaction, etc.) has a List/Get/Put/Patch/Delete function. It follows that the more irregular REST /web API behaviors and access control checks are not going to be supported if I focus primarily on F3. I don't have a specific example in mind but I'm quite sure I'll discover some. For instance there is a single F3 set of functions for handling all kinds of attachments but there are a set of REST API endpoints for each kind of resource those attachments belong to (issue, comment, review, review comment, ...).

Calling Get(repoID, ...)(ctx) will change state on the ctx... I don't really expect unpredictable side-effects from a Get method.

Good point. It should be named CheckGet as it is equivalent to the middleware used in the API routes, which buffer data in the context to avoid repeating the same operation many times. But I did not think it could be used in the context of the REST / web API routes and the current implementation / architecture is not a good fit.

Thanks a lot for showing me that the direction I was heading to was not good 🙏 I'm still unsure if creating a generic permission system is the solution but it is worth exploring further. I'll work on it and, if you don't mind, I'll ping you again when there is something concrete to look at.

For archive, the description of this design as it is today.

The purpose of this design is to define how to enforce permissions when F3 is used to mirror a remote forge to a local forge.

For instance, mirroring https://codeberg.org/forgejo/design to a Forgejo instance running from source can be done with:

curl -sS -H "Authorization: token $F3_LOCAL_TOKEN" --json '{"url":"/forge/organizations/6/projects/115/issues/42/comments/1978 "token": "'$F3_CODEBERG_ORG_TOKEN'", "type": "forgejo", "from": "/forge/organizations/forgejo/projects/design"}' http://0.0.0.0:3000/api/v1/f3

The Forgejo F3 driver based on gof3 performs the following:

• Use the API with the F3_CODEBERG_ORG_TOKEN token to read from the https://codeberg.org/forgejo/design repository
• Create a F3 archive in a directory, as defined by F3
• Read those files and modify the Forgejo database and files (just as a series of API call would) to create or update the local copy of the design repository

Since it is not using the API, each operation of this internal driver must verify that the doer (i.e. the user used to create the F3_LOCAL_TOKEN token in the example above) has the required permissions (for instance the author of an issue may not be allowed to set the labels).

Implementation

Draft

Note

drafts are developed and published in the forgefriends repository

• API endpoint

Proposed Design

The /api/v1/f3 endpoint to create or update the mirror of a remote repository is given:

• The URL of the remote forge (e.g. /forge/organizations/6/projects/115/issues/42/comments/1978
• The F3 path to mirror on the remote forge (e.g. /forge/users/myuser/projects/myproject)
• A token with enough permissions to read the repository from the remote forge

The gof3 package implements the high level mirroring logic and relies on:

• the drivers provided by the gof3 package to read the remote forge. The permissions are enforced by the remote forge and is not in scope for this discussion.
• the F3 internal driver to write to the local Forgejo instance.

This driver provides the following functions for each resource:

• List (e.g. listing all comments of an issue in a way similar to GET /repos/{owner}/{repo}/issues/comments)
• Get (e.g. read a comment in a way similar to GET /repos/{owner}/{repo}/issues/comments/{id})
• Put (e.g. create a comment in a way similar to POST /repos/{owner}/{repo}/issues/{index}/comments)
• Patch (e.g. edit a comment in a way similar to PATCH /repos/{owner}/{repo}/issues/comments/{id})
• Delete (e.g. delete a comment in a way similar to DELETE /repos/{owner}/{repo}/issues/comments/{id})

Each driver function call is responsible to enforce the required permissions.

Enforcing permissions

The Forgejo API and web permissions are not always implemented in functions or methods common to both that F3 could re-use to ensure consistency. In addition, there is a single API endpoint for F3 mirroring which means that verifications inserted when building the API routes cannot be re-used because they private functions (e.g. commentAssignment).

To keep the permission code and logic duplication to a minimum, it is implemented as follows:

• The F3 context is created in the the function that implements the /api/v1/f3 API endpoint and is inserted in the context. It serves the same purpose as APIContext: it is a cache for permission related data.
• The F3 driver functions verify permissions by calling helpers that rely on the F3 Context to figure out if it can proceed (e.g. Get on an issue comment checks if the commend ID is related and if the doer has permission).
• If the permission check fails, it panics with a F3 driver specific error and aborts the mirror.
• The panic is recovered and the error translated to a Forbidden / 403 error.

An example of permission logic duplication that cannot be avoided (without a refactoring the current Forgejo permissions architecture) is the content of the function checking the doer can read a comment.

An example of permission logic reuse is the function that calculates the permissions of the doer for a given repository. A helper of the F3 context can be used by F3 driver functions to figure out of the operation is permitted.

Absolute ID scoping

Every F3 resource (e.g. issue, comment, ...) is guaranteed to have a parent. When the ID of the resource is absolute (e.g. comment), it can be verified to belong to the parent.

For instance, the Get function on a comment verifies the ID is related by using the issue ID obtained from the parent in the F3 tree.

It is equivalent to what commentAssignment does for comment related API endpoints.

Overview of Forgejo API permissions

The permissions (aka access modes) associated to an action (API or web) of a given doer are calculated to either be read, write, admin, owner or none.

Within a repository, permissions are automatically denied if the associated Unit is disabled (e.g. if issues are disabled on a repository). For repositories that belong to an organization, team members permissions may be further modified for Code, Issues, Pull Request, Releases. Other units are not currently relevant to F3: Project, Package, Actions, nor is Wiki. If the doer is a collaborator of a given repository, its permissions are futher modified (and supersedes team permissions) to be read, write or none.

An admin user is automatically granted the highest permissions on the instance. The anonymous user and the Ghost user are not in scope because the F3 API requires that a token is provided. The token created on behalf of the Forgejo Actions user is restricted to act on the repository for which it was generated.

When an absolute ID is unrelated to its parents (e.g. a comment ID unrelated to a comment), access is denied as if it did not exist (i.e. it is 404 instead of 403).

Scopes

The token used to authentify the doer may further limit the permissions granted to the user that owns it with the following scopes:

• organization
• issue
• repository
• user

And other scopes that are not currently relevant for F3 (misc, activitypub, notifications, package).

Repository Units

The API routes have nested checks verifying the features (units) related to a resource (issue, release, etc.) are enabled. For instance mustEnableIssuesOrPull

forbids using any routes under /api/v1/repos/{owner}/{reponame}/issues unless the issues unit is set on a repository

Once the router checks have passed, it is the responsibility of each REST API endpoint to enforce additional restrictions

based on the permissions granted to the doer.

Absolute ID scoping

Some API routes contain identifiers that are relative to their parent (e.g. the index of an issue in /api/v1/repos/{owner}/{reponame}/issues/{index}). Others contain identifiers that are absolute (e.g. the id of a comment in /api/v1/repos/{owner}/{reponame}/issues/comments/{id}). An attempt to use an absolute ID unrelated to the resource (e.g. a repository) must be forbidden as it would escalate the permissions of the doer (e.g. read a comment from any private repository).

For instance commentAssignment

forbids using a comment ID if it does not belong to the related repository

Doer

The doer of an action can either be:

• A registered user
• A system user (Ghost or Forgejo Actions)
• Anonymous (user is nil)

The Forgejo Actions automatic token is bound to the Forgejo Actions system user and subject to special restrictions on all REST API endpoints under /api/v1/repos/{owner}/{repo}.

• It is denied access (no read, no write) to repositories that are no the same as the repository where the actions is running
• It has read permissions on all units if the the action is running on a pull request and the head if it is from a forked repository
• Otherwise it has write permissions on all units in the repository

The description was updated to reflect the new approach and I'll work in this direction now.

A refactor of the high level mirror logic is complete. I will now try to build on it and add more permission checks.

A refactor of the high level mirror logic is complete. I will now try to build on it and add more permission checks.

It will take another few weeks.

I wish it was faster.

Analysis of the existing permissions check when creating, editing and deleting a comment, via the REST API or the Web UI.

Web UI

POST /{username}/{reponame}/{type:^(issues|pulls)$}/{index}/comments

repo.NewComment

Fails if

• !ctx.IsSigned || (ctx.Doer.ID != issue.PosterID && !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull))
• issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) && !ctx.Doer.IsAdmin

Routes

• repo.MustAllowUserComment
• context.RepoMustNotBeArchived
• context.RepoAssignment
• verifyAuthWithOptions options.SignInRequired

POST /{username}/{reponame}/{type:^(issues|pulls)$}/{index}/comments/{id}

repo.UpdateCommentContent

Fails if

• comment.Issue.RepoID != ctx.Repo.Repository.ID
• !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull))
• !comment.Type.HasContentSupport()

Routes

• context.RepoMustNotBeArchived
• context.RepoAssignment
• verifyAuthWithOptions options.SignInRequired

Middlewares

Data retrieval

context.RepoAssignment

It collects, computes and caches a large number of permission related information but does not verify any.

Features

context.RepoMustNotBeArchived

The IsArchived field of the repository is false.

Permissions

repo.MustAllowUserComment

• checkIssueRights via GetActionIssue

checkIssueRights

For a pull request, check if the pull request unit has read permissions
For an issue, check if the issue unit has read permissions

verifyAuthWithOptions

Base function of the middlewares:

• reqSignIn
• reqSignOut
• ignSignIn / optSignIn
• ignExploreSignIn

REST API

POST /repos/{owner}/{repo}/issues/{index}/comments

repo.CreateIssueComment

Fails if:

• !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull)
• issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) && !ctx.IsUserSiteAdmin()

Routes

• mustNotBeArchived
• reqToken
• mustEnableLocalIssuesIfIsIssue
• mustEnableIssuesOrPulls
• checkTokenPublicOnly
• repoAssignment
• tokenRequiresScopes auth_model.AccessTokenScopeCategoryIssue

GET /repos/{owner}/{repo}/issues/{index}/comments

repo.ListIssueComments

Fails if:

• !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull)

Routes

• mustEnableLocalIssuesIfIsIssue
• mustEnableIssuesOrPulls
• checkTokenPublicOnly
• repoAssignment
• tokenRequiresScopes auth_model.AccessTokenScopeCategoryIssue

GET /repos/{owner}/{repo}/issues/comments/{id}

repo.GetIssueComment

Fails if:

• comment.Type != issues_model.CommentTypeComment

Routes

• commentAssignment
• mustEnableIssuesOrPulls
• checkTokenPublicOnly
• repoAssignment
• tokenRequiresScopes auth_model.AccessTokenScopeCategoryIssue

DELETE /repos/{owner}/{repo}/issues/comments/{id}

repo.DeleteIssueComment

Fails if:

• !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull))
• comment.Type != commentType

Routes

• reqToken
• commentAssignment
• mustEnableIssuesOrPulls
• checkTokenPublicOnly
• repoAssignment
• tokenRequiresScopes auth_model.AccessTokenScopeCategoryIssue

Middlewares

Data retrieval

The details of which data is retrieved and cached is skipped. Only authorization related
logic is explained, when relevant.

repoAssignment

If the token is an action token associated with a different repository than the one specified in the parameters, fail with a NotFound error.
If the token is an action token from a forked pull request, read access only, othewise write access to the repository.
Unit permissions are obtained from the token scope, unless the token is set to other repositories (access_model.GetUserRepoPermissionWithReducer)

Features

mustNotBeArchived

The IsArchived field of the repository is false.

mustEnableLocalIssuesIfIsIssue

Check that the issue unit is enabled or that the index parameter matches
the index of an existing pull request in the repository.

mustEnableIssuesOrPulls

Check that:

• The repository has the issue unit enabled and the token has read permissions AND
• The repository is a mirror or is empty (Repository.CanEnablePulls()) OR
  The repository has the pull request unit enabled and the token has read permissions

Permissions

reqToken

Check that a token is provided by the caller (not anymous, etc.).

tokenRequiresScopes

Check that the scopes given in argument are present in the token and have the required access level:

• POST, PUT, PATCH, DELETE: auth_model.Write
• otherwise: auth_model.Read

checkTokenPublicOnly

If the token has a public_only scope, check that all repository, organization, user or package found in the context are publicly available.

commentAssignment

Fail if the id parameter is for an issue that does not belong to the repository.

• comment.Issue == nil || comment.Issue.RepoID != ctx.Repo.Repository.ID
• !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull)

After writing the detailed analysis related to permission checks, I was discouraged by the amount of work necessary to do the same for all API endpoints required by F3. And quite skeptical about the maintainability of the result. And very pessimistic about the chances of anyone having time to review the changes required for the associated pull requests to enter the codebase.

But today I had an epiphany and there may be a solution that is still significant work but much more likely to succeed and be maintainable. In a nutshell:

• Ignore the web API entirely, focus on implementing F3 permissions using REST API functions
• Do not introduce new permission check functions, re-use the REST API middleware permission check functions
• Pair each F3 API endpoint with a REST API endpoint
• Automate the verification that a F3 API endpoint uses the same permission check functions, in the same order, as the matching REST API endpoint

The epiphany is that the sequence of permission checks scattered in the routes of the REST API are strictly equivalent to the same permissions checks done in the same order for an equivalent operation in F3.

To help maintain consistency with the REST API, the permissions check functions can collect and store the sequence of their calls when running tests so that they can be compared to be the same as when called from the F3 tests.

There will still be some code duplication but this is much more promising and I'll go in this direction.

@mfenniak since you've recently worked in this area of the code, I hope you will have time for a quick review of this draft pull request. In a nutshell it will allow F3 to re-use most of the permission check logic from the REST API. 🙏

Thanks to the permission related middleware distinctive signature, the list of middleware applied for a given REST API route is calculated when in test mode and can then be used by F3 functions to check the same permission checks are applied and fail if they are different.

method POST, pattern /repos/{username}/{reponame}/issues/{index}/reactions, permissions v1.checkTokenPublicOnly,v1.mustEnableIssuesOrPulls,v1.reqToken, function repo.PostIssueReaction
method DELETE, pattern /repos/{username}/{reponame}/issues/{index}/reactions, permissions v1.checkTokenPublicOnly,v1.mustEnableIssuesOrPulls,v1.reqToken, function repo.DeleteIssueReaction
method GET, pattern /repos/{username}/{reponame}/issues/{index}/assets, permissions v1.checkTokenPublicOnly,v1.mustEnableIssuesOrPulls, function repo.ListIssueAttachments
method POST, pattern /repos/{username}/{reponame}/issues/{index}/assets, permissions v1.checkTokenPublicOnly,v1.mustEnableIssuesOrPulls,v1.reqToken,v1.mustNotBeArchived, function repo.CreateIssueAttachment
method GET, pattern /repos/{username}/{reponame}/issues/{index}/assets/{attachment_id}, permissions v1.checkTokenPublicOnly,v1.mustEnableIssuesOrPulls, function repo.GetIssueAttachment
method PATCH, pattern /repos/{username}/{reponame}/issues/{index}/assets/{attachment_id}, permissions v1.checkTokenPublicOnly,v1.mustEnableIssuesOrPulls,v1.reqToken,v1.mustNotBeArchived, function repo.EditIssueAttachment
method DELETE, pattern /repos/{username}/{reponame}/issues/{index}/assets/{attachment_id}, permissions v1.checkTokenPublicOnly,v1.mustEnableIssuesOrPulls,v1.reqToken,v1.mustNotBeArchived, function repo.DeleteIssueAttachment

The refactor with the new design is completed and I have a good feeling about it. I have updated the description accordingly, the significant change are copied below.

The Forgejo REST API permissions are implemented in functions or methods that F3 must re-use to avoid code duplication. There are dozens of REST API endpoints and the permissions they enforce are tested by a number of tests. It would be difficult to re-create such tests for permission checks created specifically for F3. They would also be difficult to maintain.

There is a single API endpoint for F3 mirroring. It requires a PAT that must be used by the F3 driver before performing an operation.

For instance, the Get() driver function to read a repository must perform the same permission checks as the GET /repos/{owner}/{repo} REST API endpoint.

To achieve this re-use:

• the middleware functions that perform permission checks are refactored to use an interface rather than a struct. The methods provided by the interface are exclusively used for data storage. The logic is unchanged. See the refactored RepoAccess middleware for an example.
• the F3 function calls the REST API permission checks in the same order they are specified in the routes of the corresponding REST API endpoint. See how the Get() driver function to read a repository calls RepoAccess and CheckTokenPublicOnly in the same order the REST API route to GET /repos/{owner}/{repo} calls them as specified by the route that leads to the endpoint.

All F3 function are paired with a REST API endpoint and automated tests verify that both run the same permission checks in the same order. Figuring out which permissions checks are performed by a given REST API endpoint is not trivial. The automated tests that fail when the checks change in the REST API are essential to the maintainability of F3. It would otherwise require a scrutiny that cannot reasonably be expected from maintainers. See how the sequence of permissions checks are harvested for each REST API call when running tests and verified to match what F3 functions do by collecting calls and matching them.

I realized today that the refactoring of the REST API middlewares could be quite beneficial to Forgejo independent of F3. Because it creates the conditions to improve permissions checks testing. I moved this particular discussion in its own design unit. I have explored and implemented enough edge cases by now to be convinced this is a good path forward.

Implementing the REST API permissions check tests is an extra effort but I'm happy to do it because:

• It is doable in a finite amount of time (there are not too many middleware)
• Since it benefits Forgejo, It is much more likely to be merged than a refactor that only benefits F3
• It does not introduce any functional changes

#63

For the record, here is the description of this design as it was before the Facilitate and improve the testing of the REST API permissions checks design was implemented in June 2026.

curl -sS -H "Authorization: token $F3_LOCAL_TOKEN" --json '{"url":"https://codeberg.org", "token": "'$F3_CODEBERG_ORG_TOKEN'", "type": "forgejo", "from": "/forge/organizations/forgejo/projects/design"}' http://0.0.0.0:3000/api/v1/f3

The description was updated to take into account that the Facilitate and improve the testing of the REST API permissions checks design is implemented in Forgejo and F3 can make use of the facility it provides which consists of pairing a F3 operation with a REST API endpoint to verify that both rely on the same sequence of permissions functions.

F3 needs additional testing to complement the MustMatch function because the permission functions are only as good as the parameters they are given. The next steps of this design will be to:

• Implement permission tests as part of the F3 compliance suite. It currently does not provide anything of the sort and assumes all permission checks are the responsibility of the forge and not the driver. This is a fair assumption when dealing with a remote forge. But Not for a driver that runs inside the forge itself.
• Pair each driver function with their REST API equivalent. There will surely be exceptions that do not quite fit. Or others that require extracting the permission logic from an existing REST API handler (such as chore: add coverage for REST API fork destination permission check)