Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

abandoned end-to-end role test #1609

Closed
wants to merge 11 commits into from
Closed

abandoned end-to-end role test #1609

wants to merge 11 commits into from

Conversation

davepacheco
Copy link
Collaborator

This seems weird but I'm creating this PR as a record of something I tried and abandoned. This way when I'm tempted to try it again (or ask why we didn't do it this way) I can go read why not.

This PR was an attempt to create an end-to-end test that verifies the behavior of authz roles. The idea was to create an actual resource hierarchy using the external API (so, creating a real Organization, Project, Instance, etc.), assign every possible role on each resource in the hierarchy (using separate users), and verify what API calls each user was able to do. This sounds great, but it has a bunch of problems:

  • it's hard to test "DELETE" because, well, you're deleting part of the resource hierarchy that you then need to recreate
  • it's hard to test the endpoints that modify the policy itself
  • it takes a very long time
  • this test doesn't tell us that each role grants the right permissions per se, but rather whether each role was able to successfully complete each API operation. On the one hand, that's more important: it's no help if we implement the permissions correctly but the API call doesn't check them correctly. On the other hand, it's possible for the API call to succeed or fail by accident -- e.g., it might fail because it's checking some different permission that the caller also didn't have.

While writing this test, I decided a better approach is to decompose this into two or three problems:

  1. does each role grant precisely the permissions that we expect? I implemented this under add unit test for authz policy #1123. This test literally just does the authz check for every action on every resource, using a bunch of users representing every possible role.
  2. does each API call check precisely the permissions that we expect? This is covered by need better testing around minimum privileges #1374 (not yet implemented).
  3. for each API call, for each permission that it checks, does it bail out as expected when that permission is missing? There's not even a ticket for this one yet.

With the first two things tested, we can have a lot of confidence that we're correctly enforcing the authz policy. There are a few advantages to breaking things down this way:

  • In part (1), It's easy to test "Delete" and "ModifyPolicy" because we're not actually deleting or modifying anything -- we're just doing the authz checks.
  • I said above that the test in this PR takes a long time. That's partly because of the exhaustive nature of the test: it requires (number of resources) times (number of actions) checks. But for each check, we're doing all the work required for an API call just to verify the authorize() bit. Plus we have to do all the database operations to set up (and tear down and re-set-up) the resource hierarchy in the first place. By separating it as proposed, part (1) doesn't do any of that extra work. It just does the authz check for each user and resource (rather than a whole API call) and it doesn't need to set up or tear down any database resources.
  • Take the case I mentioned above where the end-to-end test succeeds or fails by accident: this can't happen with this approach because we're testing the underlying pieces (the permissions and the checks), not just the end result.

There may still be room for an end-to-end test ((3) above). For example, it's conceivable that an implementation passes both of the first two tests but that the API call does not correctly bail out on some particular authz check. (e.g., if somebody called authorize() but forgot the ? after it). This test could probably be limited to verifying only failure cases, which makes things a lot easier. Much (but not all) of this is in turn covered by the existing "unauthorized" test.

@davepacheco davepacheco mentioned this pull request Aug 17, 2022
71 tasks
@davepacheco
Copy link
Collaborator Author

I forgot to mention: there are a bunch more bug fixes in this PR. I pulled those out and landed them in #1110. The only (potentially) interesting part of this PR at this point is the new integration test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant