Pull request: forgejo/forgejo#12512/files
The situation
The REST API permissions checks are partly implemented in middlewares inserted in the routes leading to each endpoint. In addition the function implementing the endpoint sometimes implements permissions checks of their own. A number of tests are implemented as integration tests that create various scenarios to verify the permissions are enforced when a REST API call happens, for instance when trying to access an issue in a private repository. The test coverage for these checks improved over time, most notably when a security bug is fixed.
The problem
With hundreds of REST API endpoints and the current strategy, implementing exhaustive tests ensuring the permissions checks for each REST API endpoint are enforced would require creating variations (private repository, etc.) for each of them. It is:
- Exhausting for the developer (identifying which variations are relevant for a given REST API endpoint is tedious and error prone)
- Very challenging to maintain (when a new permission check is added, the existing tests may lack coverage)
- More difficult to fix permissions enforcement related security fixes (tests must be added to cover all related REST API endpoints)
Because of this, the odds that some permissions are not properly enforced is higher than it should be if testing REST API permissions checks was easier.
The solution
The short version is:
- Move the permission checks that are embedded in functions in middlewares into their own package
- Add minimal tooling to make it significantly easier to test the functions in this package compared to how it is currently done
- Write tests and fixtures to improve test coverage
The two key elements of this solution are to:
- Reduce the space to test from hundreds of REST API endpoints to a few dozen middlewares
- Create only meaningfull variations, based on the sequence of middleware created by the routes
Implementation scope and future steps
- Implement the design for all the middleware found in the REST API routes file.
- Convert permissions checks into specific middleware and add tests (fork destination check for instance)
- When reviewing and implementing features that require new permission checks, isolate them in the permission package instead of inlining them in the function or creating a middleware
Technical details
See the pull request implementing this design at forgejo/forgejo#12512/files
Collecting middleware sequences from routes
It happens in testing only and is built in memory when the test or the server is initialized.
limiting-factor/forgejo@156e999ed9/modules/web/route.go (L114)
For instance,
DELETE /repos/{username}/{reponame}/issues/{index}/pin implemented by function repo.UnpinIssue is checked with the middlewares (in that order):
v1.tokenRequiresScopes Write Issuev1.repoAccessv1.checkTokenPublicOnlyv1.mustEnableIssuesOrPullsv1.mustEnableLocalIssuesIfIsIssuev1.reqToken
Fixtures for testing a permission function
For each permission function backing the corresponding middleware (for instance ReqOrgOwnership)
Lines 12 to 39 in limiting-factor/forgejo@216f2d8
funcReqOrgOwnership(ctxContext){ifIsUserSiteAdmin(ctx){return}varorgIDint64ifctx.GetOrg()!=nil{orgID=ctx.GetOrg().ID}elseifctx.GetTeam()!=nil{orgID=ctx.GetTeam().OrgID}else{ctx.Error(http.StatusInternalServerError,"","reqOrgOwnership: unprepared context")return}isOwner,err:=organization.IsOrganizationOwner(ctx.GetContext(),orgID,ctx.GetDoer().ID)iferr!=nil{ctx.Error(http.StatusInternalServerError,"IsOrganizationOwner",err)return}elseif!isOwner{ifctx.GetOrg()!=nil{ctx.Error(http.StatusForbidden,"","Must be an organization owner")}else{ctx.NotFound()}return}}a set of fixtures is registered to create all the scenario to provide full coverage of the function.
Lines 48 to 60 in limiting-factor/forgejo@216f2d8
fixtures:[]*fixtureType{{data:newFixtureData(map[string]string{"org":"ReqOrgOwnershipOrg","setOrg":"true",}),},{data:newFixtureData(map[string]string{"doer":"doeradmin","setOrg":"true",}),},Testing
A single test:
- Collect all middleware sequences created when building the routes
- For each permission middleware loop over all the fixtures that were registered to assert their result
- For each sequence run the permission checks in the expected order to assert the sequence can be successfully run to completion
Lines 214 to 236 in limiting-factor/forgejo@216f2d8
funcTestAPIv1Permissions(t*testing.T){defertest.MockVariableValue(&setting.Service.DefaultAllowCreateOrganization,true)()defertest.MockVariableValue(&setting.IsInTesting,true)()defertest.MockVariableValue(&setting.DisableGitHooks,false)()unittest.PrepareTestEnv(t)// because setting.IsInTesting == true, it will record the// middleware sequence of each route it buildsapiv1.Routes()buildSignatureStringToFunctionTest(t)runs:=0t.Logf("running all fixtures for each permission function")for_,sequence:=rangegetPermissionSequencesForFunctions(t){runs+=testSequence(t,sequence,false)}t.Logf("verify all unique permission sequences can run successfully")uniqueSequences:=apiv1_permissions_tests.GetUniquePermissionsSequences()for_,sequence:=rangeuniqueSequences{runs+=testSequence(t,sequence,true)}**Pull request:** https://codeberg.org/forgejo/forgejo/pulls/12512/files
## The situation
The REST API permissions checks are partly implemented in middlewares inserted in the routes leading to each endpoint. In addition the function implementing the endpoint sometimes implements permissions checks of their own. A number of tests are implemented as integration tests that create various scenarios to verify the permissions are enforced when a REST API call happens, for instance when trying to access an issue in a private repository. The test coverage for these checks improved over time, most notably when a security bug is fixed.
## The problem
With hundreds of REST API endpoints and the current strategy, implementing exhaustive tests ensuring the permissions checks for each REST API endpoint are enforced would require creating variations (private repository, etc.) for each of them. It is:
- Exhausting for the developer (identifying which variations are relevant for a given REST API endpoint is tedious and error prone)
- Very challenging to maintain (when a new permission check is added, the existing tests may lack coverage)
- More difficult to fix permissions enforcement related security fixes (tests must be added to cover all related REST API endpoints)
Because of this, the odds that some permissions are not properly enforced is higher than it should be if testing REST API permissions checks was easier.
## The solution
The short version is:
- Move the permission checks that are embedded in functions in middlewares into their own package
- Add minimal tooling to make it significantly easier to test the functions in this package compared to how it is currently done
- Write tests and fixtures to improve test coverage
The two key elements of this solution are to:
- Reduce the space to test from hundreds of REST API endpoints to a few dozen middlewares
- Create only meaningfull variations, based on the sequence of middleware created by the routes
## Implementation scope and future steps
1. [Implement the design](https://codeberg.org/forgejo/forgejo/pulls/12512/files
) for all the middleware found in the REST API routes file.
1. Convert permissions checks into specific middleware and add tests (fork destination check for instance)
1. When reviewing and implementing features that require new permission checks, isolate them in the permission package instead of inlining them in the function or creating a middleware
## Technical details
See the pull request implementing this design at https://codeberg.org/forgejo/forgejo/pulls/12512/files
### Collecting middleware sequences from routes
It happens in testing only and is built in memory when the test or the server is initialized.
https://codeberg.org/limiting-factor/forgejo/src/commit/156e999ed9de2dd639d8f2cdc1f732ce8538e87a/modules/web/route.go#L114
For instance,
`DELETE /repos/{username}/{reponame}/issues/{index}/pin` implemented by function `repo.UnpinIssue` is checked with the middlewares (in that order):
- `v1.tokenRequiresScopes Write Issue`
- `v1.repoAccess`
- `v1.checkTokenPublicOnly`
- `v1.mustEnableIssuesOrPulls`
- `v1.mustEnableLocalIssuesIfIsIssue`
- `v1.reqToken`
### Fixtures for testing a permission function
For each permission function backing the corresponding middleware (for instance ReqOrgOwnership)
https://codeberg.org/limiting-factor/forgejo/src/commit/216f2d81fac39f9e9c0d43e98210b357cdf137e2/routers/api/v1/permissions/req_org_ownership.go#L12-L39
a set of fixtures is registered to create all the scenario to provide full coverage of the function.
https://codeberg.org/limiting-factor/forgejo/src/commit/216f2d81fac39f9e9c0d43e98210b357cdf137e2/routers/api/v1/permissions/tests/req_org_ownership_test.go#L48-L60
### Testing
A single test:
- Collect all middleware sequences created when building the routes
- For each permission middleware loop over all the fixtures that were registered to assert their result
- For each sequence run the permission checks in the expected order to assert the sequence can be successfully run to completion
https://codeberg.org/limiting-factor/forgejo/src/commit/216f2d81fac39f9e9c0d43e98210b357cdf137e2/routers/api/v1/permissions/tests/functions_test.go#L214-L236