feat: add limits and tier to jwt claim #1382
Conversation
claim decoding was only tested in an ignored test. I also added an assertion about the account tier of the decoded claim
oddgrd
changed the title
common/src/claims.rs
Outdated
| } | ||
|
|
||
| pub trait ClaimExt { | ||
| fn project_limit(&self) -> u32; |
There was a problem hiding this comment.
It would be better to have this method as can_create_project() to prevent the logic from spilling into the handlers (controllers).
There was a problem hiding this comment.
It would also be good to put this trait in a separate file.
There was a problem hiding this comment.
It would also be good to put this trait in a separate file.
Why?
can_create_project
Are you saying that the business logic should live in that function? Then it has to take in current_project_count or something.
There was a problem hiding this comment.
I implemented these changes last night: f900d1a
It feels more organized, but it meant needing to make some changes to the test code in gateway.
There was a problem hiding this comment.
@jonaro00 to have the business logic in one file. Thereby being easier to update 😄
common/src/claims.rs
Outdated
| } | ||
|
|
||
| pub trait ClaimExt { | ||
| fn project_limit(&self) -> u32; |
There was a problem hiding this comment.
It would also be good to put this trait in a separate file.
Why?
can_create_project
Are you saying that the business logic should live in that function? Then it has to take in current_project_count or something.
additionally, check if a user can create a project in the create_project handler
| project_name: ProjectName, | ||
| account_name: AccountName, | ||
| is_admin: bool, | ||
| limit: Option<u32>, | ||
| can_create_project: bool, | ||
| idle_minutes: u64, | ||
| ) -> Result<FindProjectPayload, Error> { |
There was a problem hiding this comment.
Technically this function should not take in an argument to check if a project can be created. I think it is fine for now since I don't see an easy way to move the logic to the caller.
gateway/src/service.rs
Outdated
| /// Verify that the account's current project count is lower than its limit. | ||
| async fn can_create_project( | ||
| service: Arc<GatewayService>, | ||
| account_name: &AccountName, | ||
| is_admin: bool, | ||
| ) -> bool { | ||
| if is_admin { | ||
| return true; | ||
| }; | ||
|
|
||
| let project_count = service | ||
| .get_project_count(account_name) | ||
| .await | ||
| .expect("to get the account's project count"); | ||
|
|
||
| project_count < MAX_PROJECTS_DEFAULT | ||
| } |
There was a problem hiding this comment.
Ideally tests should not have logic that replicates the code being tested. Can we not just pass true and false into the test arguments?
There was a problem hiding this comment.
Replicating the logic means your test won't identify any errors in the replicated code.
There was a problem hiding this comment.
I didn't want to remove our only test of project limits, but I guess replicating the logic here does not resolve that. I think this should rather be tested at a higher level (handler tests), since it's now checked there. Thanks! I will remove this change.
There was a problem hiding this comment.
I added a new test for project limits. These tests could use some more work I think, but they're not run in CI, and we may be rewriting them for the new orchestrator anyway.
Description of change
Add a
Limitsstruct that will hold all the account limits to the JWTClaim, and also add theAccountTier(moved to shuttle-common) to theClaim, so we can use it in instrumentation. I left adding it to instrumentation out of this PR, that will be done in a follow-up.I used an extension trait for the methods to extract the limits from the
Claim. This allows us to have the methods for extracting limits from the claim only available where they are needed. An alternative would be to implement the methods on Claim directly, or make the fields ofClaimpublic, but that would make us more exposed to breaking changes. I'm not sure that breaking changes will be an issue here, though, since we create the limits from the tier. Thoughts on this subject are very welcome!How has this been tested? (if applicable)
Updated the
authtest to verify that the limits and tier are correctly set for an admin and for a basic user, and verified that the gateway tests that test project limit still pass.Additionally, I tested deploying a hello-world and postgres project to staging on an old deployer, as well as a new deployer (after deploying this branch to staging).