Skip to content

Improve RBAC comprehensive testing #5204

Closed
@Emyrk

Description

@Emyrk

Goal

Robust authorization testing to prevent security holes created by developer error. Reduce chance developer forgets to call Authorize, uses wrong rbac object/action, or a role is granted more privilege than it should have.

Current State

RBAC testing is broken up into 2 parts to achieve comprehensive tests across all the api routes.

  1. Assert various roles against given action+object combinations (link)
  • ✔️ owner, orgMemberMe, orgAdmin, templateAdmin
  • memberMe, otherOrgAdmin, otherOrgMember, userAdmin
  1. Assert a route checks a given action+object pair (link)

This makes testing a "route" with various user role combinations much simpler. Writing a unit test to check various roles against an endpoint is tedious and long. The above two unit tests give a high degree of confidence the correct rbac assertion will be made on the given route, and we know the user role combinations will return a true or false to that assertion.

Improve unit tests

Assert various roles

The current set of users and objects were created ad hoc. We should make sure all combinations of expected objects + actions that are hit in prod are covered.

Assert route checks

The current route check assertions are a bit weak.

Only asserts 1 rbac check per route

Firstly, only the last rbac assertion is checked. Some routes run 2 rbac checks (assuming the first one passed) (example). We need to have a way to assert all the checks that are run. Possibly requiring some design to allow the endpoint to run until the second Authorize check (as all the auth calls return false atm).

Idea (From @spikecurtis): If we have a route that calls authorize N times, we should run the assertion test on the route N times. Each call should have authorize fail on the Nth call.

Example: Route postWorkspacesByOrganization calls Authorize on create.workspace and read.template.

  1. Test will run 2 times on this route.
  2. 1st test runs and fails on create.workspace. Assert checks run assuming only 1 authorize call
  3. 2nd test runs and succeeds on create.workspace. Route continues and fails on read.template. Assert checks run assuming 2 authorize calls.

This is to ensure the route always fails as Unauthorized if any of the individual authorize calls returns false.

Asserts weakly

The object assertions do not check any fields not set. So for the workspace endpoint we do not check the Owner, OrgID, ACLUserList or ACLGroupList fields of the object. We only check the Type. We should assert exact matches.

Does not assert the caller's roles

We should also asser the roles of the user calling in this test. A non-admin user should be used. This would catch things where the wrong user is being asserted as the actor in cases where more than 1 user is involved. (eg creating a workspace for another user)

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions