-
-
Notifications
You must be signed in to change notification settings - Fork 359
Make Permissions into one table #1919
Comments
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 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. |
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. 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. |
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 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.
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 |
That sounds reasonable. It's anyway currently not possible to use (add) permission with the same name to both |
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
The text was updated successfully, but these errors were encountered: