Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

Make Permissions into one table #1919

Open
Sboonny opened this issue Nov 17, 2022 · 5 comments
Open

Make Permissions into one table #1919

Sboonny opened this issue Nov 17, 2022 · 5 comments
Labels
Discussion Ideas, feature requests, views on features. Anything which is a discussion.

Comments

@Sboonny
Copy link
Member

Sboonny commented Nov 17, 2022

Discuss your ideas or share your views:

while discussion chapter permission with Oliver, we noticed that the instance.permission and chapter.permission are confusing as they push for wrong impressions of the data nature

We can merge the permissions and roles into their own big table for simpler readability and easier maintenance

@Sboonny Sboonny added the Discussion Ideas, feature requests, views on features. Anything which is a discussion. label Nov 17, 2022
@ojeytonwilliams
Copy link
Contributor

ojeytonwilliams commented Nov 17, 2022

To elaborate: there's nothing special about instance_permissions vs chapter_permissions from an authorization perspective. They're all lumped together in https://github.com/freeCodeCamp/chapter/blob/c3e4b1b2b760fe75d92b3e5cf45c6c5bb2798aec/common/permissions.ts#L1-L33, and we only care about the Permission object.

So, there's no obvious reason why they should have separate tables in the database. That makes it harder to work with and obscures the fact that they're really the same types of thing. It's particularly confusing because it looks like an instance owner can't have ChapterPermissions, but they can (and do).

The difference comes from the type of role. i.e. if an instance_role has a permission, then users with that role have that permission for every chapter, whereas if a chapter_role has the same permission, it's tied to a particular chapter.

@allella
Copy link
Contributor

allella commented Nov 17, 2022

I feel like we started out with fewer tables and one of the fCC folks, I think Tom, did a full rework and that's when these got introduced in #958, so I don't remember a specific reason or conversation about that need.

#967 (comment)

I do recall we separated the user permissions and preferences into different tables as an added layer of security since we didn't want people will lesser permissions being able to have read or write address to a table that could allow for a potential escalation of a role or permissions.

@gikf
Copy link
Member

gikf commented Nov 18, 2022

In case of throwing all roles together. How would be differentiated between role that's a chapter role - specific for single chapter only - and an instance role?

@ojeytonwilliams
Copy link
Contributor

I do recall we separated the user permissions and preferences into different tables as an added layer of security since we didn't want people will lesser permissions being able to have read or write address to a table that could allow for a potential escalation of a role or permissions.

I believe that was the rationale. However, the reduced complexity probably outweighs that since complexity invites bugs. Not that this is desperately complex, just more than it needs to be.

In case of throwing all roles together. How would be differentiated between role that's a chapter role - specific for single chapter only - and an instance role?

I don't think we'd want to throw the roles together, just the permissions. We'd definitely have a hard time writing code to differentiate them.

So, in this schema we'd still have instance_roles, instance_role_permissions, chapter_roles, chapter_role_permissions, event_roles and event_role_permissions. The difference is that we'd replace the three x_permissions table with a single permissions table.

Since the instance_permission chapter-edit really is the same thing as the chapter_permission chapter-edit we end up cutting down on data duplication and reduce the complexity of migrations.

@Sboonny Sboonny changed the title Make Roles into one table and Permissions into one table Make Permissions into one table Nov 18, 2022
@gikf
Copy link
Member

gikf commented Nov 18, 2022

That sounds reasonable. It's anyway currently not possible to use (add) permission with the same name to both InstancePermission and ChapterPermission.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Discussion Ideas, feature requests, views on features. Anything which is a discussion.
Projects
None yet
Development

No branches or pull requests

4 participants