Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 4, 2024

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Jun 3, 2024

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.

@Emyrk Emyrk changed the title chore: typed database custom permissions chore: implement typed database for custom permissions Jun 3, 2024
Comment on lines +1788 to +1790
SitePermissions CustomRolePermissions `db:"site_permissions" json:"site_permissions"`
OrgPermissions CustomRolePermissions `db:"org_permissions" json:"org_permissions"`
UserPermissions CustomRolePermissions `db:"user_permissions" json:"user_permissions"`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So much better than json.RawMessage

@Emyrk Emyrk requested a review from johnstcn June 3, 2024 21:54
Copy link
Member

@johnstcn johnstcn left a 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.

Comment on lines +1 to +3
-- 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.
Copy link
Member

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.

Copy link
Member Author

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.

@Emyrk Emyrk changed the title chore: implement typed database for custom permissions chore: implement typed database for custom permissions (breaks existing custom roles) Jun 4, 2024
@Emyrk Emyrk merged commit e320661 into main Jun 4, 2024
36 checks passed
@Emyrk Emyrk deleted the stevenmasley/custom_roles_dbtype branch June 4, 2024 14:27
@github-actions github-actions bot locked and limited conversation to collaborators Jun 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants