Simplify security.rs and authorization logic #674
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
svix-gabriel
changed the title
svix-gabriel
force-pushed
the
gabriel/update-auth
branch
2 times, most recently
from
0cf9c9a
to
3cc63b4
Compare
svix-dylan
reviewed
Oct 19, 2022
svix-dylan
approved these changes
Oct 19, 2022
Some specifics * Many of our extractors had redundant information, or largely unused fields. This PR removes them * The `Permissions` struct has been simplified a bit, namely by moving things into an enum (as opposed to using the `type_` enum just as a tag. * Removed the axum extractor for `Permissions`. We often chain authorization logic on top of the `Permissions`. However, the axum extractor for `Permissions` only authenticates requests, it doesn't authorize them. * Fixes a discrepancy between `list_event_types` and `get_event_type`. `list_event_types` was not checking for Organization privileges like `get_event_types` was. * `AuthenticatedOrganization`, `AuthenticatedApp`, etc, were also simplified a bit, and moved into a separate `permissions.rs` module.
svix-gabriel
force-pushed
the
gabriel/update-auth
branch
from
3cc63b4
to
cc01b84
Compare
svix-gabriel
added a commit
that referenced
this pull request
Dec 22, 2022
This PR fixes a regression with the `GET /event-type` endpoint. ## Motivation In [#674](cc01b84#diff-2a76cfc85cbfeae62fdc056d281dfa9a9c61fdfcb434435b02993b5cd31ddf58), we refactored a lot of our permission logic so that the `Permission` struct was no longer extracted directly in axum handlers. Instead, a `permission::<Struct>` is extracted instead, which describes the resources the user has authorized access too. The regression was introduced [here](cc01b84#diff-2a76cfc85cbfeae62fdc056d281dfa9a9c61fdfcb434435b02993b5cd31ddf58L176). Originally, `GET /event-type` accepted the `Permission` struct directly - which effectively means any authenticated user has access, but it was changed to `permissions::Organization`, which [enforces org-only access](cc01b84#diff-6482eed99ed190ec2d1e3da2390b61e530eb26e6f8bd4d24cfa5149a95401e77R33). ## Solution Update the `GET /event-type` to accept any authenticated user. Conceptually this maps to `permissions::ReadAll`, which is equivalent to the original code where the `Permission` struct was extracted directly in the Axum handler.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors
security.rsand our auth* related axum extractors.Motivation
Solution
Some specifics
Permissionsstruct has been simplified a bit, namely by moving things into an enum (as opposed to using thetype_enum just as a tag.Permissions. We often chain authorization logic on top of thePermissions. However, the axum extractor forPermissionsonly authenticates requests, it doesn't authorize them.list_event_typesandget_event_type.list_event_typeswas not checking for Organization privileges likeget_event_typeswas.AuthenticatedOrganization,AuthenticatedApp, etc, were also simplified a bit, and moved into a separatepermissions.rsmodule.