- Notifications
You must be signed in to change notification settings - Fork927
chore: implement typed database for custom permissions (breaks existing custom roles)#13457
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
replacing the previous raw json.
| SitePermissions CustomRolePermissions `db:"site_permissions" json:"site_permissions"` | ||
| OrgPermissions CustomRolePermissions `db:"org_permissions" json:"org_permissions"` | ||
| UserPermissions CustomRolePermissions `db:"user_permissions" json:"user_permissions"` |
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.
So much better thanjson.RawMessage
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.
LGTM 👍 but probably good to call out that this will invalidate any existing custom roles.
| -- Previous custom roles are now invalid, as the json changed. Since this is an | ||
| -- experimental feature, there is no point in trying to save the perms. | ||
| -- This does not elevate any permissions, so it is not a security issue. |
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.
Might be no harm to call this out in case adventurous folks are already playing around with this.
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 added it in the PR description and the commit.
e320661 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Custom roles in the db were raw json. Using sqlc overrides, I can make these typed which is much better. This is just a refactor. Fixed up the tests to match.
Forgot this was something we could easily do. It removes some converting that we used to have to do.
Any existing custom org roles will be replaced with 0 permissions. This is a change in format.