-
Notifications
You must be signed in to change notification settings - Fork 38
feat: Enforce api keys #479
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
base: main
Are you sure you want to change the base?
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
use relayer_macros::require_permissions; | ||
|
||
/// List plugins | ||
#[require_permissions(["plugins:get:all"])] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
example of regular permission (no parameters)
} | ||
|
||
/// Calls a plugin method. | ||
#[require_permissions(["plugins:execute:{plugin_id}"])] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example of scoped permission, (plugin_id parameter), an API key that passes this authorization must contain a permission of the form:
plugins:execute:abc-abc-abc-abc
AND the plugin_id
passed as function parameter must be: abc-abc-abc-abc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work!
In general i like the idea of macro at route level.
In the future for some advanced cases we would need standard methods to invoke to check permissions logic but for most cases macros would do the job.
I think we should just see how permissions should be defined.
I have added my two cents in the comments. In general i think permissions should be simple and we can define scope outside of permission string and use in the logic to check. This would eliminate need to define multiple permissions, * and for specific id and would keep permission system simpler.
fn test_macro_detects_template_parameters() { | ||
// This is a compile-time test - if the macro compiles, it works | ||
let tokens = quote::quote! { | ||
#[require_permissions(["relayers:get:{relayer_id}"])] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can it work with multiple permissions
For example required permissions are to get specific relayer or to be able to get any relayer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works with multiple ones, but in this specific case relayers:get:{relayer_id}
should match both a specific or all relayers (using wildcards).
- If API key contains
relayers:get:abc-abc-abc-abc
permission, then it passes. - If API key contains
relayers:get:*
permission, then also passes.
I think there might be endpoints where we have more than one as you said, e.g. if you want to enforce both signing:execute:{relayer_id}
+ transactions:execute:{relayer_id}
in the same endpoint:
#[require_permissions(["signing:execute:{relayer_id}", "transactions:execute:{relayer_id}"])]
should work
} | ||
|
||
/// Get API key permissions | ||
#[require_permissions(["api_keys:get:{api_key_id}"])] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one api key can have api_keys:get:some_id
some key can have access to all. is that api_keys:get:all?
if we want OR condition are we defining both permissions like:
#[require_permissions(["api_keys:get:{api_key_id}", "api_keys:get:all"])]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in case you want to have some user accessing all the api keys you will use a wildcard.
similar to what I mentioned here
The :all
suffix in permissions corresponds to the permission to list, rather to access all (this is in cases where listing does not include all the information of each entry). But I see it creates some confusion, maybe replacing ..:get:all
by ..:list:all
in the action makes more sense
} | ||
|
||
/// Calls a plugin method. | ||
#[require_permissions(["plugins:execute:{plugin_id}"])] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of plugins, only execute plugin permissions are needed?
We do not check resources used in plugin logic?
For example plugin sends transaction via some relayer. Does relayer permission needs to be there or just plugin perm is enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this was the most complicated part, the challenge there is that we don't really control the plugin interactions at "routes" level, Maybe doing some extra validation inside the relayer-api
in /plugins
works
fn test_macro_detects_template_parameters() { | ||
// This is a compile-time test - if the macro compiles, it works | ||
let tokens = quote::quote! { | ||
#[require_permissions(["relayers:get:{relayer_id}"])] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we keep permissions simple like
notifications:list
notifications:read
notifications:create
notifications:update
notifications:delete
and then define scope for permissions which can be either global or list of ids.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way we are keeping permissions simple, for example relayers:read, and later in the logic we check scope of that permission which can be either global or list of items.
That way we do not need to tie permissions with specific entities.
for example api route can just be docorated with simple permission relayers:read. If api key has that permission our logic will check for permission scope which is defined at api key create phase.
Macro fingerprint would needed to be slightly modified, for example
#[require_permissions(["relayers:read"], "relayer_id")]
permission struct example:
{
"action": "relayers:read",
"resource": { "ids": ["*"] },
}
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree! that makes it simpler :)
Summary
This PR introduces a new proc macro for endpoints
require_permissions
that has a list of string literals as parameters indicating the permissions that the API key must contain for calling the endpoint.Example
The permissions created are registered under
constants/permissions.rs
, API keys will be created using some subset of those permissions, then when creating a new permission, the developer must register it underpermissions.rs
and in the endpoint where applies.Scopes
Permissions are scoped by resource (generally either relayer_id or plugin_id) like:
resource:action:scope
Examples:
Permission to execute transactions in
sepolia-example
relayer:transactions:execute:sepolia-example
Permission to list all relayers:
relayers:get:all
Permission to sign using
sepolia-example
relayer:signing:execute:sepolia-example
Permission to run
my-plugin
:plugins:execute:my-plugin
Testing Process
sepolia-example
&solana-example
value
, you can use this api key to loadsepolia-example
relayer:curl -X GET http://localhost:8080/api/v1/relayers/sepolia-example \ -H "Authorization: Bearer SEPOLIA_RELAYER_API_KEY_VALUE" The call should succeed.
solana-example
relayer:curl -X GET http://localhost:8080/api/v1/relayers/solana-example \ -H "Authorization: Bearer SEPOLIA_RELAYER_API_KEY_VALUE" The call should succeed.
the call should fail with an unauthorized error.
Checklist