Role validation - allow permissionId to be optional #14853
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
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
mike12345567
commented
Oct 23, 2024
| @@ -214,8 +215,8 @@ export function roleValidator() { | |||
| }).optional(), | |||
| // this is the base permission ID (for now a built in) | |||
| permissionId: Joi.string() | |||
| .valid(...Object.values(permissions.BuiltinPermissionID)) | |||
| .required(), | |||
| .valid(...Object.values(BuiltinPermissionID)) | |||
There was a problem hiding this comment.
One of two code changes in this PR.
mike12345567
commented
Oct 23, 2024
| @@ -134,7 +135,13 @@ export async function save(ctx: UserCtx<SaveRoleRequest, SaveRoleResponse>) { | |||
| } | |||
| // assume write permission level for newly created roles | |||
| if (isCreate && !permissionId) { | |||
| permissionId = PermissionLevel.WRITE | |||
| permissionId = BuiltinPermissionID.WRITE | |||
There was a problem hiding this comment.
Other Code change in this PR.
samwho
approved these changes
Oct 23, 2024
adrinr
reviewed
Oct 23, 2024
| @@ -50,7 +51,7 @@ export class Role implements RoleDoc { | |||
| _id: string | |||
| _rev?: string | |||
| name: string | |||
| permissionId: string | |||
| permissionId: BuiltinPermissionID | |||
There was a problem hiding this comment.
Will this not be used for custom roles, as well?
There was a problem hiding this comment.
Yes, this is used in all roles, but its never a string - its just a very old type, we've always validated the Role to make sure it is one of the values in BuiltinPermissionID.
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.
Description
Frontend now expects to be able to supply role without a
permissionId, this update makes it so that the validators allow this.Added test cases for the behaviour - also realised the types in this area were pretty incorrect based on the validators,
permissionIdwas just astringwhen it was actually typed, fixed this.