[FEAT] Concept for user roles #1887

fnetX commented 2023年12月05日 21:44:12 +01:00

Needs and benefits

For large Forgejo instances, managing user roles is a highly relevant feature. For example, it would be great to allow people to "moderate" on Codeberg, but potentially limit the impact that moderators can do.

This matter is also relevant for the Forgejo-in-German-schools project, see forgejo/discussions#71 for details. There is likely human power / funding available to get this started, but it is important to draft a concept so that the work can be accepted upstream. I recommend that the chance is taken to get work in this domain "sponsored".

The specific need for this project is:

• there are admins with full admin rights
• there are school-admins with limited admin rights (probably "moderators" who can add and lock users etc, but not toggle any "dangerous" instance-wide settings)
• different roles that allow creating repos and organizations (i. e. disallow certain users to create organizations)
• in the context of the project it is okay to have a backend interface to get user roles, it is likely okay to maintain a soft-fork for certain logic like "if userRole is foo, then do not display bar" in templates or in functions

For Codeberg and potentially other public community-maintained instances it will likely be good to have a moderator role that can perform certain actions, but is also not able to behave destructive towards the instance.
It could also be used to build a reputation system, e.g. automatically give users a more trusted role after being member for a while or performing certain steps, and limiting access to expensive features (code search, CI, more storage etc) for new users.

Feature Description

I'd like to discuss a strategy to implement a backend and coordinate the development between Forgejo and potential developers of the Forgejo-BW project.

I have the following strategies in mind:

Super simple:

• the per-user key/value setting table receives a "role" setting per user
• the role is represented by a string
• the user API is allowed to set / get the field per user (during creation and update)
• an internal function is added to query the role per user

Ideally, there is:

• an instance-wide list of roles available for internal use, and the settings are validated against this
• a dropdown in the user list allows selecting from the available roles
• this could be realized like this:
  • there is a pre-defined list of roles like "admin", "moderator", "user", "guest" in Forgejo with pre-defined access roles
  • a setting alike "CUSTOM_ROLES" allows a comma-separated list of (either additional or overriding) roles that can be selected
  • adding a custom role there means that it receives "normal" user rights by default and custom patches in a soft fork must make use of the role

Fully-featured:

• roles become an integral part of Forgejo
• a table tracks the relation between users and their roles
• there is a way to define exact permissions per role, e.g.
  • role "moderator" can view, lock and delete repos, but not view user details
  • role "support" can view repos but not modify / delete, but can view user details
• a list of important permissions is implemented and checked, and can be extended step-by-step later

Semi-flexible:

• another inspiration could be the permission system used by the Matrix messenger for example
• user accounts receive a permission level as value between 0 and 100 (or a different scale if desired)
• permission checks can be implemented to require permissions of at least (>=) the necessary value
• permissions do not necessarily need to be named, but it should be easy to allow instance admins to define a list of aliases (e.g. >= 20 is "moderator")
• the system appears to have a good "effort/benefit" ratio, but it explicitly does not support the use case with different roles for user support and moderation as described above
• we should probably gather more feedback on how well it works for Matrix, but additional upsides are that changing the role model later usually does not require "migrating" the permissions, because it is simply a threshold that can be compared against; it feels rather simple but could be too limited for certain use cases

Screenshots

No response

### Needs and benefits For large Forgejo instances, managing user roles is a highly relevant feature. For example, it would be great to allow people to "moderate" on Codeberg, but potentially limit the impact that moderators can do. This matter is also relevant for the Forgejo-in-German-schools project, see https://codeberg.org/forgejo/discussions/issues/71 for details. There is likely human power / funding available to get this started, but it is important to draft a concept so that the work can be accepted upstream. I recommend that the chance is taken to get work in this domain "sponsored". The specific need for this project is: - there are admins with full admin rights - there are school-admins with limited admin rights (probably "moderators" who can add and lock users etc, but not toggle any "dangerous" instance-wide settings) - different roles that allow creating repos and organizations (i. e. disallow certain users to create organizations) - in the context of the project it is okay to have a backend interface to get user roles, it is likely okay to maintain a soft-fork for certain logic like "if userRole is foo, then do not display bar" in templates or in functions For Codeberg and potentially other public community-maintained instances it will likely be good to have a moderator role that can perform certain actions, but is also not able to behave destructive towards the instance. It could also be used to build a reputation system, e.g. automatically give users a more trusted role after being member for a while or performing certain steps, and limiting access to expensive features (code search, CI, more storage etc) for new users. ### Feature Description I'd like to discuss a strategy to implement a backend and coordinate the development between Forgejo and potential developers of the Forgejo-BW project. I have the following strategies in mind: **Super simple:** - the [per-user key/value setting table](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/models/user/setting.go) receives a "role" setting per user - the role is represented by a string - the user API is allowed to set / get the field per user (during creation and update) - an internal function is added to query the role per user Ideally, there is: - an instance-wide list of roles available for internal use, and the settings are validated against this - a dropdown in the user list allows selecting from the available roles - this could be realized like this: - there is a pre-defined list of roles like "admin", "moderator", "user", "guest" in Forgejo with pre-defined access roles - a setting alike "CUSTOM_ROLES" allows a comma-separated list of (either additional or overriding) roles that can be selected - adding a custom role there means that it receives "normal" user rights by default and custom patches in a soft fork must make use of the role **Fully-featured:** - roles become an integral part of Forgejo - a table tracks the relation between users and their roles - there is a way to define exact permissions per role, e.g. - role "moderator" can view, lock and delete repos, but not view user details - role "support" can view repos but not modify / delete, but can view user details - a list of important permissions is implemented and checked, and can be extended step-by-step later **Semi-flexible:** - another inspiration could be the permission system used by the Matrix messenger for example - user accounts receive a permission level as value between 0 and 100 (or a different scale if desired) - permission checks can be implemented to require permissions of at least (>=) the necessary value - permissions do not necessarily need to be named, but it should be easy to allow instance admins to define a list of aliases (e.g. >= 20 is "moderator") - the system appears to have a good "effort/benefit" ratio, but it explicitly does not support the use case with different roles for user support and moderation as described above - we should probably gather more feedback on how well it works for Matrix, but additional upsides are that changing the role model later usually does not require "migrating" the permissions, because it is simply a threshold that can be compared against; it feels rather simple but could be too limited for certain use cases ### Screenshots _No response_

fnetX commented 2024年01月10日 10:52:03 +01:00

After thinking about the topic, and having a few discussions with other developers and admins, this is what I propose. I believe that the proposal is sensible to approach a full permission system. It is a long way, though, and it will likely require a lot of time before the flexibility will benefit the general public.
I further expect challenges in upstreaming the work to Gitea, but I think we should not consider this a blocker and see where forgejo/discussions#96 leads.

First Pull Request

• adds a column that assigns integer or unsigned integer ids to users
• sets a default power level for new users (e.g. 10 or 100) to reserve lower ids for restrictions
• adds the necessary internal API to check user power levels (e.g. to hide information in templates, or have permission checks in backend)
• exposes power levels via the external API (user creation, ...)

Usefullness on its own: This allows for keeping the portions of the development small. It will also allow the Forgejo-in-German-schools project and Codeberg to make preliminary experiences with the system using custom patches or templates.

Migration of existing permissions

This needs a little consideration, because if this work is not upstreamed to Gitea, it will create a divergence between the two. The migration still allows Forgejo to replace Gitea in the near future, however, there is no switch back

• the User struct fields IsAdmin, IsRestricted, IsActive and ProhibitLogin are migrated to the new permission system, e.g.
  • IsAdmin: 1000
  • ProhibitLogin: 0 or -1
  • IsRestricted: 5
  • IsActive: 100 (default for users who successfully signed up and activated)
• special care needs to go to the case where a user is created as "restricted", but not activated, or the same applies to the prohibit login options, because they are not exclusive in the existing permission system
  • I believe it is not a problem to make the levels mutually exclusive when only an admin can modify them
  • an alternative could be to use numbers to represent combinations, e.g. ProhibitLogin&IsActive is a different number than ProhibitLogin&IsRestricted
• internal functions are updated to use the new system, it would basically require using a function instead of user.IsAdmin, alternatively the struct fields could be kept (but the colums in the database removed), and generated when getting the user (but IMHO more prone to potential mistakes)

Usefullness on its own: The refactor allows for future flexibility and makes sense to split the development work. Tests that verify the permissions of users who have been assigned admin permissions the same way as before can help against security regressions.

Demo UI and first actual use

To users, the benefit of the new system was not apparent so far. I propose to add a first actual feature on top of it:

• a global "Moderator" role is added with elevated permissions (to be decided)
• there is a Admin UI to manage role labels (pre-filled with e.g. User: 100, Moderator: 500, Admin: 1000) and allows admins to add and rename roles
  • depending on whether the power levels should remain static in code, maybe it should not be possible to modify the existing roles
• admin can select user role in dropdown or modify number manually (likely input box which proposes labels when typing, but also accepts numbers)
• user roles are displayed in profile, so that custom roles serve the purpose of allowing to label users (e.g. "Support")
• the web API also accepts power levels as labels

Usefullness on its own: Allows setting custom labels to indicate who can be reached (e.g. Support). Since permissions below a certain level are not allowed to sign in, it can allow instance administrators to have different roles for retired users (e.g. "FormerEmployee", "Banned", ...) that all result in the same permissions.

Custom permission mapping

In the future, there could be ways for instance admins to map custom permissions to roles.

• relevant places that are useful for admins to configure are represented to the admin (permission name + input/dropdown for power level)
• a database table holds the permission name and required power level
• for performance, the easiest way is likely to query permissions on application startup → permission changes require restart
• this could e.g. restrict repo creation, private repos, using packages etc to certain power levels
• this could also supersede the "Restricted user" level that currently exists, which does not seem to be widely in use (the behaviour could be reproduced by allowing instance admins to make exploring instance-wide repos require a certain permission level, and grant restricted users a lower value)

Automatic User/Trust system

In the context of moderation, this has been discussed a lot, often citing Discourse forums as a source for inspiration

• the power level for new users is lowered
• the user automatically levels up in a predefined range
  • criteria needs to be chosen, e.g. age of account, number of interactions
• the increasing power level unlock more "trust" / features

After thinking about the topic, and having a few discussions with other developers and admins, this is what I propose. I believe that the proposal is sensible to approach a full permission system. It is a long way, though, and it will likely require a lot of time before the flexibility will benefit the general public. I further expect challenges in upstreaming the work to Gitea, but I think we should not consider this a blocker and see where https://codeberg.org/forgejo/discussions/issues/96 leads. **First Pull Request** - adds a column that assigns integer or unsigned integer ids to users - sets a default power level for new users (e.g. 10 or 100) to reserve lower ids for restrictions - adds the necessary internal API to check user power levels (e.g. to hide information in templates, or have permission checks in backend) - exposes power levels via the external API (user creation, ...) Usefullness on its own: This allows for keeping the portions of the development small. It will also allow the Forgejo-in-German-schools project and Codeberg to make preliminary experiences with the system using custom patches or templates. **Migration of existing permissions** This needs a little consideration, because if this work is not upstreamed to Gitea, it will create a divergence between the two. The migration still allows Forgejo to replace Gitea in the near future, however, there is no switch back - the User struct fields IsAdmin, IsRestricted, IsActive and ProhibitLogin are migrated to the new permission system, e.g. - IsAdmin: 1000 - ProhibitLogin: 0 or -1 - IsRestricted: 5 - IsActive: 100 (default for users who successfully signed up and activated) - special care needs to go to the case where a user is created as "restricted", but not activated, or the same applies to the prohibit login options, because they are not exclusive in the existing permission system - I believe it is not a problem to make the levels mutually exclusive when only an admin can modify them - an alternative could be to use numbers to represent combinations, e.g. ProhibitLogin&IsActive is a different number than ProhibitLogin&IsRestricted - internal functions are updated to use the new system, it would basically require using a function instead of user.IsAdmin, alternatively the struct fields could be kept (but the colums in the database removed), and generated when getting the user (but IMHO more prone to potential mistakes) Usefullness on its own: The refactor allows for future flexibility and makes sense to split the development work. Tests that verify the permissions of users who have been assigned admin permissions the same way as before can help against security regressions. **Demo UI and first actual use** To users, the benefit of the new system was not apparent so far. I propose to add a first actual feature on top of it: - a global "Moderator" role is added with elevated permissions (to be decided) - there is a Admin UI to manage role labels (pre-filled with e.g. User: 100, Moderator: 500, Admin: 1000) and allows admins to add and rename roles - depending on whether the power levels should remain static in code, maybe it should not be possible to modify the existing roles - admin can select user role in dropdown or modify number manually (likely input box which proposes labels when typing, but also accepts numbers) - user roles are displayed in profile, so that custom roles serve the purpose of allowing to label users (e.g. "Support") - the web API also accepts power levels as labels Usefullness on its own: Allows setting custom labels to indicate who can be reached (e.g. Support). Since permissions below a certain level are not allowed to sign in, it can allow instance administrators to have different roles for retired users (e.g. "FormerEmployee", "Banned", ...) that all result in the same permissions. **Custom permission mapping** In the future, there could be ways for instance admins to map custom permissions to roles. - relevant places that are useful for admins to configure are represented to the admin (permission name + input/dropdown for power level) - a database table holds the permission name and required power level - for performance, the easiest way is likely to query permissions on application startup → permission changes require restart - this could e.g. restrict repo creation, private repos, using packages etc to certain power levels - this could also supersede the "Restricted user" level that currently exists, which does not seem to be widely in use (the behaviour could be reproduced by allowing instance admins to make exploring instance-wide repos require a certain permission level, and grant restricted users a lower value) **Automatic User/Trust system** In the context of moderation, this has been discussed a lot, often citing Discourse forums as a source for inspiration - the power level for new users is lowered - the user automatically levels up in a predefined range - criteria needs to be chosen, e.g. age of account, number of interactions - the increasing power level unlock more "trust" / features

rdwz commented 2024年01月14日 17:37:14 +01:00

This needs a little consideration, because if this work is not upstreamed to Gitea, it will create a divergence between the two. The migration still allows Forgejo to replace Gitea in the near future, however, there is no switch back

it's a fucking useless loophole! now is the time to go into that proposed "beyond coding" 😘

> This needs a little consideration, because if this work is not upstreamed to Gitea, it will create a divergence between the two. The migration still allows Forgejo to replace Gitea in the near future, however, there is no switch back it's a fucking useless loophole! now is the time to go into that proposed "beyond coding" 😘

algernon commented 2024年01月14日 20:12:52 +01:00

I have yet to read everything in full, but wanted to express a concern with one of the basic building blocks of the proposal:

• adds a column that assigns integer or unsigned integer ids to users
• sets a default power level for new users (e.g. 10 or 100) to reserve lower ids for restrictions

I don't think power level based permission systems are a good thing. They seem and feel simple if you look from afar, but there are situations that are very hard or confusing to express in such a system.

This problem is nicely summed up later:

• special care needs to go to the case where a user is created as "restricted", but not activated, or the same applies to the prohibit login options, because they are not exclusive in the existing permission system
  • I believe it is not a problem to make the levels mutually exclusive when only an admin can modify them
  • an alternative could be to use numbers to represent combinations, e.g. ProhibitLogin&IsActive is a different number than ProhibitLogin&IsRestricted

It simply isn't a flexible system, it makes it hard and confusing to implement adjacent, non-exclusive permissions. I found it very awkward to work with such systems in the past.

Off the top of my head, I'd suggest another approach, one that still allows storing the role as a single integer, but its meaning is much more flexible. If we extended the current permission system to be more fine grained, like split the "admin" permissions up into smaller sets, and allow not only none/read/write permissions, but - say - delete too, we could express things like "permission to delete comments, but not modify them", and so on. Code and templates would care about these permissions, not the roles.

Roles would be a mapping of a role id to a set of permissions, and a name. So a "moderator" role would have read/delete (or read/write) permissions on admin:users, read/delete on comments, and so on. Similarly, a "Restricted" role could have very little permissions, and an "Inactive" role likewise - but a different small set. Both would be clearly expressible, without any special handling.

The templates and most of the code would need to know nothing about the roles. Pretty much the only place that would is the User model, which would load the permissions too, which the rest of the code would work with.

This would also allow us to have custom roles, without modifying the code or the templates. With the roles being available to templates, it would still allow customised templates to act on the role, rather than permissions, if so need be.

I have yet to read everything in full, but wanted to express a concern with one of the basic building blocks of the proposal: > - adds a column that assigns integer or unsigned integer ids to users > - sets a default power level for new users (e.g. 10 or 100) to reserve lower ids for restrictions I don't think power level based permission systems are a good thing. They seem and feel simple if you look from afar, but there are situations that are *very* hard or confusing to express in such a system. This problem is nicely summed up later: > - special care needs to go to the case where a user is created as "restricted", but not activated, or the same applies to the prohibit login options, because they are not exclusive in the existing permission system > - I believe it is not a problem to make the levels mutually exclusive when only an admin can modify them > - an alternative could be to use numbers to represent combinations, e.g. ProhibitLogin&IsActive is a different number than ProhibitLogin&IsRestricted It simply isn't a flexible system, it makes it hard and confusing to implement adjacent, non-exclusive permissions. I found it very awkward to work with such systems in the past. Off the top of my head, I'd suggest another approach, one that still allows storing the role as a single integer, but its meaning is much more flexible. If we extended the current permission system to be more fine grained, like split the "admin" permissions up into smaller sets, and allow not only none/read/write permissions, but - say - delete too, we could express things like "permission to delete comments, but not modify them", and so on. Code and templates would care about these permissions, not the roles. Roles would be a mapping of a role id to a set of permissions, and a name. So a "moderator" role would have `read/delete` (or `read/write`) permissions on `admin:users`, `read/delete` on comments, and so on. Similarly, a "Restricted" role could have very little permissions, and an "Inactive" role likewise - but a different small set. Both would be clearly expressible, without any special handling. The templates and most of the code would need to know nothing about the roles. Pretty much the only place that would is the User model, which would load the permissions too, which the rest of the code would work with. This would also allow us to have custom roles, without modifying the code or the templates. With the roles being available to templates, it would still allow customised templates to act on the role, rather than permissions, if so need be.

fnetX commented 2024年01月22日 17:44:28 +01:00

@algernon I am not sure if I understand your proposal correctly. You want to assign users a role as an "id", and another table maps the role_id to a set of permissions (e.g. read/write on x, read on private repos, none on instance settings to not leak passwords, etc etc)?

I somehow wonder about the performance implications of realizing this and querying the permission set, and viable caching strategies. I believe that we should keep it as simple as possible by keeping the amount of units that allow custom permission mapping to a bare minimum.

An alternative with much less flexibility, but likely easier to implement, could be to include a static set of permissions in Forgejo and assign them to users.

Possible elevated permissions with custom mapping (None/Read/Write/Delete each, unless noted otherwise):

• (future) instance-wide announcements (x)
• Admin Dashboard (Read = Server Stats, Write = allow to trigger actions) (x)
• admin/auths
• admin/identity (organizations, user accounts, emails) (x)
• admin/code assets (read would include private repos)¹ (x)
• admin/integrations
• admin/configuration
• admin/system notices
• admin/monitoring

(x = relevant for a first implementation)

---

Alternative with fixed permission sets:

• Admin: all permissions
• Owner: Potentially for hosted Forgejo versions, can modify settings, but certain technical settings like database connections are restricted
• Support: Admin permissions on users and repos
• Moderator: Admin permissions on repos, ideally with a way to disable users (without reading more information about them)
• Trusted/Helper: Possibly elevated privileges on an instance in the future
• Default: current user
• ... more roles with added restrictions

---

The second option (going with fixed permission levels) sounds much easier to implement. The code would likely prominently list the roles which have access to a route or certain settings and could thus be patched by users if needed.

@algernon I am not sure if I understand your proposal correctly. You want to assign users a role as an "id", and another table maps the role_id to a set of permissions (e.g. read/write on x, read on private repos, none on instance settings to not leak passwords, etc etc)? I somehow wonder about the performance implications of realizing this and querying the permission set, and viable caching strategies. I believe that we should keep it as simple as possible by keeping the amount of units that allow custom permission mapping to a bare minimum. An alternative with much less flexibility, but likely easier to implement, could be to include a static set of permissions in Forgejo and assign them to users. Possible elevated permissions with custom mapping (None/Read/Write/Delete each, unless noted otherwise): - (future) instance-wide announcements **(x)** - Admin Dashboard (Read = Server Stats, Write = allow to trigger actions) **(x)** - admin/auths - admin/identity (organizations, user accounts, emails) **(x)** - admin/code assets (read would include private repos)[^1] **(x)** - admin/integrations - admin/configuration - admin/system notices - admin/monitoring (**x** = relevant for a first implementation) [^1]: I wonder if it makes sense here to allow write without read. The use case could be as follows: Allow a team to apply repo flags on the instance (e.g. missing license), but don't allow them to read private content they don't otherwise have access to) --- Alternative with fixed permission sets: - Admin: all permissions - Owner: Potentially for hosted Forgejo versions, can modify settings, but certain technical settings like database connections are restricted - Support: Admin permissions on users and repos - Moderator: Admin permissions on repos, ideally with a way to disable users (without reading more information about them) - Trusted/Helper: Possibly elevated privileges on an instance in the future - Default: current user - ... more roles with added restrictions --- The second option (going with fixed permission levels) sounds much easier to implement. The code would likely prominently list the roles which have access to a route or certain settings and could thus be patched by users if needed.

algernon commented 2024年01月22日 18:53:23 +01:00

@algernon I am not sure if I understand your proposal correctly. You want to assign users a role as an "id", and another table maps the role_id to a set of permissions (e.g. read/write on x, read on private repos, none on instance settings to not leak passwords, etc etc)?

Either that, or move the mapping to the configuration, so User would have a RoleID, and the lookup would be something like permissions := setting.Roles.roles[user.RoleID]. No extra table or lookup necessary then, at the cost of having to map roles out in the config, and not being able to easily change them at run-time.

This would still allow us to move the role -> permission mapping to the database later on, if there's a need for that, and if the performance hit turns out to be acceptable.

I somehow wonder about the performance implications of realizing this and querying the permission set, and viable caching strategies.

The role -> permission table would be tiny, my gut feeling is that the lookup would only have a negligible cost. But we don't need a table, we can put the mapping in the app settings too, initially (see above).

I believe that we should keep it as simple as possible by keeping the amount of units that allow custom permission mapping to a bare minimum.

I agree, but I stand by my opinion that if we change the permission system, we should change it to something that is easy to extend later, without redoing everything all over again.

An alternative with much less flexibility, but likely easier to implement, could be to include a static set of permissions in Forgejo and assign them to users.

That's... not far away from my proposal! The permissions in my scheme would be static. The difference is between assigning N permissions to a user, or a single role with a role->N permissions mapping somewhere. Otherwise we're pretty much on the same page.

I think that role->permission mapping is more extensible, and it can be implemented without a noticable performance hit.

Possible elevated permissions with custom mapping (None/Read/Write/Delete each, unless noted otherwise):
[...]

Yup, these are the kind of permissions I had in mind.

• (future) instance-wide announcements (x)
• Admin Dashboard (Read = Server Stats, Write = allow to trigger actions) (x)
• admin/auths
• admin/identity (organizations, user accounts, emails) (x)
• admin/code assets (read would include private repos)[^1] (x)
• admin/integrations
• admin/configuration
• admin/system notices
• admin/monitoring

(x = relevant for a first implementation)

[^1]: I wonder if it makes sense here to allow write without read. The use case could be as follows: Allow a team to apply repo flags on the instance (e.g. missing license), but don't allow them to read private content they don't otherwise have access to)

I think allowing write access without read makes sense. However, if either read or write has access to private stuff, so should the other, or there should be separate read/write permissions for public and private things.

Alternative with fixed permission sets:

• Admin: all permissions
• Owner: Potentially for hosted Forgejo versions, can modify settings, but certain technical settings like database connections are restricted
• Support: Admin permissions on users and repos
• Moderator: Admin permissions on repos, ideally with a way to disable users (without reading more information about them)
• Trusted/Helper: Possibly elevated privileges on an instance in the future
• Default: current user
• ... more roles with added restrictions

These would be perfect built-ins for a scheme where the role->permission mappings are in the configuration file =)

---

The second option (going with fixed permission levels) sounds much easier to implement. The code would likely prominently list the roles which have access to a route or certain settings and could thus be patched by users if needed.

Easier to implement, but - quoting myself -:

It simply isn't a flexible system, it makes it hard and confusing to implement adjacent, non-exclusive permissions. I found it very awkward to work with such systems in the past.

I don't think it's that much easier (if easier at all!) to implement power levels than more finer grained permissions and a role->permissions map. Forgejo already has a permission system that is close-ish to my proposal, all it needs is a few more permissions for finer control, and an update to callsites to do checks based on Role, rather than Permission, letting the Role handle the permission check underneath. Switching to a "power level" or "trust level" based system would require more changes, as far as I see.

> @algernon I am not sure if I understand your proposal correctly. You want to assign users a role as an "id", and another table maps the role_id to a set of permissions (e.g. read/write on x, read on private repos, none on instance settings to not leak passwords, etc etc)? Either that, or move the mapping to the configuration, so `User` would have a `RoleID`, and the lookup would be something like `permissions := setting.Roles.roles[user.RoleID]`. No extra table or lookup necessary then, at the cost of having to map roles out in the config, and not being able to easily change them at run-time. This would still allow us to move the role -> permission mapping to the database later on, if there's a need for that, and if the performance hit turns out to be acceptable. > I somehow wonder about the performance implications of realizing this and querying the permission set, and viable caching strategies. The role -> permission table would be *tiny*, my gut feeling is that the lookup would only have a negligible cost. But we don't *need* a table, we can put the mapping in the app settings too, initially (see above). > I believe that we should keep it as simple as possible by keeping the amount of units that allow custom permission mapping to a bare minimum. I agree, but I stand by my opinion that if we change the permission system, we should change it to something that is easy to extend later, without redoing everything all over again. > An alternative with much less flexibility, but likely easier to implement, could be to include a static set of permissions in Forgejo and assign them to users. That's... not far away from my proposal! The *permissions* in my scheme would be static. The difference is between assigning N permissions to a user, or a single role with a role->N permissions mapping *somewhere*. Otherwise we're pretty much on the same page. I think that role->permission mapping is more extensible, and it can be implemented without a noticable performance hit. > Possible elevated permissions with custom mapping (None/Read/Write/Delete each, unless noted otherwise): [...] Yup, these are the kind of permissions I had in mind. > > - (future) instance-wide announcements **(x)** > - Admin Dashboard (Read = Server Stats, Write = allow to trigger actions) **(x)** > - admin/auths > - admin/identity (organizations, user accounts, emails) **(x)** > - admin/code assets (read would include private repos)[^1] **(x)** > - admin/integrations > - admin/configuration > - admin/system notices > - admin/monitoring > > (**x** = relevant for a first implementation) > > > > `[^1]`: I wonder if it makes sense here to allow write without read. The use case could be as follows: Allow a team to apply repo flags on the instance (e.g. missing license), but don't allow them to read private content they don't otherwise have access to) I think allowing write access without read makes sense. However, if either read or write has access to private stuff, so should the other, or there should be separate read/write permissions for public and private things. > Alternative with fixed permission sets: > > - Admin: all permissions > - Owner: Potentially for hosted Forgejo versions, can modify settings, but certain technical settings like database connections are restricted > - Support: Admin permissions on users and repos > - Moderator: Admin permissions on repos, ideally with a way to disable users (without reading more information about them) > - Trusted/Helper: Possibly elevated privileges on an instance in the future > - Default: current user > - ... more roles with added restrictions These would be perfect built-ins for a scheme where the role->permission mappings are in the configuration file =) > --- > > The second option (going with fixed permission levels) sounds much easier to implement. The code would likely prominently list the roles which have access to a route or certain settings and could thus be patched by users if needed. Easier to implement, but - quoting myself -: > It simply isn't a flexible system, it makes it hard and confusing to implement adjacent, non-exclusive permissions. I found it very awkward to work with such systems in the past. I don't think it's *that* much easier (if easier at all!) to implement power levels than more finer grained permissions and a role->permissions map. Forgejo already has a permission system that is close-ish to my proposal, all it needs is a few more permissions for finer control, and an update to callsites to do checks based on Role, rather than Permission, letting the Role handle the permission check underneath. Switching to a "power level" or "trust level" based system would require more changes, as far as I see.

oliverpool commented 2024年01月23日 10:43:29 +01:00

After reading the various arguments, I have the following understanding of the suggested implementation:

• there is mapping Role -> List of permissions (this mapping could live in the database/config file/go code - config is probably a sweet spot between performance and flexibility - however )
• there is a nullable role_id to the users table (a User may have at most 1 role: just create more roles to accommodate for finer permission distribution)

The goal is that:

• all code should check if an action is allowed by checking a given permission
• the Role -> List of permissions is mainly for humans (to give meaningful names, like Teacher, Helper...)

If a reputation system is to be added at some point, it could automatically update the role of a given user based one some internal rules.

---

So one step would be to replace the 100+ usages of the IsAdmin field of the models/user.User struct with a RoleID and CanCreate/CanRead/CanUpdate/CanDelete methods (or at least check that they can be replaced).

After reading the various arguments, I have the following understanding of the suggested implementation: - there is mapping `Role -> List of permissions` (this mapping could live in the database/config file/go code - config is probably a sweet spot between performance and flexibility - however ) - there is a nullable `role_id` to the `users` table (a User may have at most 1 role: just create more roles to accommodate for finer permission distribution) The goal is that: - all code should check if an action is allowed by checking a given **permission** - the `Role -> List of permissions` is mainly for humans (to give meaningful names, like `Teacher`, `Helper`...) If a reputation system is to be added at some point, it could automatically update the role of a given user based one some internal rules. --- So one step would be to replace the 100+ usages of the `IsAdmin` field of the `models/user.User` struct with a `RoleID` and `CanCreate/CanRead/CanUpdate/CanDelete` methods (or at least check that they can be replaced).

oliverpool commented 2024年01月26日 11:06:16 +01:00

I had another thought, regarding Role -> List of permissions: the role names could match the actual names of the domain.

For instance in Codeberg:

• vorstand
• active-member
• honorary-member
• supporting-member
• moderation-team

For a university:

• teacher
• head-of-departement
• technical-support

For an association it could allow access to specific resources based on the membership.

Maybe this is too far-fetched?

I had another thought, regarding `Role -> List of permissions`: the role names could match the actual names of the domain. For instance in Codeberg: - vorstand - active-member - honorary-member - supporting-member - moderation-team For a university: - teacher - head-of-departement - technical-support For an association it could allow access to specific resources based on the membership. Maybe this is too far-fetched?

i9e1 commented 2024年01月28日 18:30:24 +01:00

I wonder if there is a need to define static "roles" at all.
What about treating a "role" just as an alias for a specific, fine-grained permission_set?

Mostly amplifying

The difference is between assigning N permissions to a user, or a single role with a role->N permissions mapping somewhere. Otherwise we're pretty much on the same page.
but with a more concrete implementation suggestion.

I would call role_id something like permission_set /permission_bitset / permission_bitset_integer and don't see why it should be nullable (each value could be interpreted as specific, maybe empty, set of permissions).
Code should check via functions like haspermission(user, p) to be as unambigous and extensible as possible.
Than permission checking and how the data is efficiently represented / stored are seperated conceptually.

By set in permission_set here I mean set in a mathematical sense or an ordered list of permissions granted/denied.
These can be represented as bitstring = integer for each user.
Each single permission must be named anyways to avoid confusion, so this is just a compact implementation.
(haspermission(user, p) checks if p is a member of user's permission set, should be straightforward with [https://pkg.go.dev/github.com/willf/bitset] with very low performance burden).

This approach would also allow for adding more permissions in the future by interpreting / using previously unused bits.
There is a tradeoff in defining the initial maximum size, I have no idea how many, [#1887 (comment)] listed 19 permissions. With a structure asread/write/delete xyz, 32bit would allow for 10 different xyz-s (comments, PRs, issues, users, repos, ...).

I somehow wonder about the performance implications of realizing this and querying the permission set, and viable caching strategies. I believe that we should keep it as simple as possible by keeping the amount of units that allow custom permission mapping to a bare minimum.

The bitset is clearly less efficient in terms of memory than numbering all valid "bare minimum" combinations, say 4 byte vs. 1 byte, but lookup should be cheaper (no indirection at all), caching irrelevant?

A potential drawback here may be permission "creep". If fine grained permissions are possible, as individual permissions, a user could end up with unusable or at least incoherent permissions like write comments but not read comments. If a hierachy is implied (as with write >= read?), coherency has to be ensured when modifying permissions for a user.
(Performance for this check should not be an issue here and this is an unfrequent tasks.)

---

Matching the roles of the domain is a UI thing for default sets of permissions and can be done as aliases e.g. via a config.
(Role -> N permissions)

---

for reference, Unix-like filesystem permission are represented similarly [https://en.wikipedia.org/wiki/File-system_permissions#Numeric_notation].

I wonder if there is a need to define static "roles" at all. What about treating a "role" just as an alias for a specific, fine-grained `permission_set`? Mostly amplifying > The difference is between assigning N permissions to a user, or a single role with a role->N permissions mapping somewhere. Otherwise we're pretty much on the same page. but with a more concrete implementation suggestion. I would call `role_id` something like `permission_set` /`permission_bitset` / `permission_bitset_integer` and don't see why it should be nullable (each value could be interpreted as specific, maybe empty, set of permissions). Code should check via functions like `haspermission(user, p)` to be as unambigous and extensible as possible. Than permission checking and how the data is efficiently represented / stored are seperated conceptually. By *set* in `permission_set` here I mean set in a mathematical sense or an ordered list of permissions granted/denied. These can be represented as bitstring = integer for each user. Each single permission must be named anyways to avoid confusion, so this is just a compact implementation. (`haspermission(user, p)` checks if `p` is a member of `user`'s permission set, should be straightforward with [https://pkg.go.dev/github.com/willf/bitset] with very low performance burden). This approach would also allow for adding more permissions in the future by interpreting / using previously unused bits. There is a tradeoff in defining the initial maximum size, I have no idea how many, [https://codeberg.org/forgejo/forgejo/issues/1887#issuecomment-1530133] listed 19 permissions. With a structure as`read/write/delete xyz`, 32bit would allow for 10 different xyz-s (comments, PRs, issues, users, repos, ...). > I somehow wonder about the performance implications of realizing this and querying the permission set, and viable caching strategies. I believe that we should keep it as simple as possible by keeping the amount of units that allow custom permission mapping to a bare minimum. The bitset is clearly less efficient in terms of memory than numbering all valid "bare minimum" combinations, say 4 byte vs. 1 byte, but lookup should be cheaper (no indirection at all), caching irrelevant? A potential drawback here may be permission "creep". If fine grained permissions are possible, as individual permissions, a user could end up with unusable or at least incoherent permissions like `write comments` but not `read comments`. If a hierachy is implied (as with `write >= read`?), coherency has to be ensured when modifying permissions for a user. (Performance for this check should not be an issue here and this is an unfrequent tasks.) --- Matching the roles of the domain is a UI thing for default sets of permissions and can be done as aliases e.g. via a config. (`Role -> N permissions`) --- for reference, Unix-like filesystem permission are represented similarly [https://en.wikipedia.org/wiki/File-system_permissions#Numeric_notation].

algernon commented 2024年01月28日 19:48:57 +01:00

I wonder if there is a need to define static "roles" at all.
What about treating a "role" just as an alias for a specific, fine-grained permission_set?

That's pretty much what I proposed with the role->permissions mapping, with "static" roles being hardcoded or in the config as a first step, and eventually moving them to the database if the performance hit is acceptable, or we found a way to make it irrelevant.

By set in permission_set here I mean set in a mathematical sense or an ordered list of permissions granted/denied.
These can be represented as bitstring = integer for each user.

I don't think it matters much how we store the permissions. Eventually, they will be turned into either an []int or a bitset or something, whichever is more practical - that's an internal implementation detail. That's not how they will be stored in the db, or in the config, so how it looks inside the app is, at this point, not very interesting. Once we know what kind of permissions we want, and how the system works, then we can talk about representing them. But even then, the first priority should be to make the system easy to understand and work with, even at the cost of performance (as long as the performance hit isn't too severe).

I somehow wonder about the performance implications of realizing this and querying the permission set, and viable caching strategies. I believe that we should keep it as simple as possible by keeping the amount of units that allow custom permission mapping to a bare minimum.

The bitset is clearly less efficient in terms of memory than numbering all valid "bare minimum" combinations, say 4 byte vs. 1 byte, but lookup should be cheaper (no indirection at all), caching irrelevant?

Lookup once the permissions are in memory is not a concern. The concern is getting the permissions into memory in the first place. Say, we have the role->permissions mapping in the database. If we need to hit the database and look up the user's permissions every time we load a user, that's going to have a performance cost. One byte, four byte, or even 128 bytes makes little difference there, because the database lookup cost is the major factor.

That's where caching comes into play: rather than looking up the role->permissions mapping on an as-needed basis, Forgejo could load them once on startup, and cache them until they're changed (and/or refresh it periodically, just not on an as-needed basis).

> I wonder if there is a need to define static "roles" at all. > What about treating a "role" just as an alias for a specific, fine-grained `permission_set`? That's pretty much what I proposed with the role->permissions mapping, with "static" roles being hardcoded or in the config as a first step, and eventually moving them to the database if the performance hit is acceptable, or we found a way to make it irrelevant. > By *set* in `permission_set` here I mean set in a mathematical sense or an ordered list of permissions granted/denied. > These can be represented as bitstring = integer for each user. I don't think it matters much how we *store* the permissions. Eventually, they will be turned into either an `[]int` or a bitset or something, whichever is more practical - that's an internal implementation detail. That's not how they will be stored in the db, or in the config, so how it looks inside the app is, at this point, not very interesting. Once we know what kind of permissions we want, and how the system works, *then* we can talk about representing them. But even then, the first priority should be to make the system easy to understand and work with, even at the cost of performance (as long as the performance hit isn't too severe). > > I somehow wonder about the performance implications of realizing this and querying the permission set, and viable caching strategies. I believe that we should keep it as simple as possible by keeping the amount of units that allow custom permission mapping to a bare minimum. > > The bitset is clearly less efficient in terms of memory than numbering all valid "bare minimum" combinations, say 4 byte vs. 1 byte, but lookup should be cheaper (no indirection at all), caching irrelevant? Lookup once the permissions are in memory is not a concern. The concern is getting the permissions into memory in the first place. Say, we have the role->permissions mapping in the database. If we need to hit the database and look up the user's permissions every time we load a user, that's going to have a performance cost. One byte, four byte, or even 128 bytes makes little difference there, because the database lookup cost is the major factor. That's where caching comes into play: rather than looking up the role->permissions mapping on an as-needed basis, Forgejo could load them once on startup, and cache them until they're changed (and/or refresh it periodically, just not on an as-needed basis).

i9e1 commented 2024年01月29日 01:23:09 +01:00

I should have called user->permission_set what you called role->permissions. Talking of roles here may be confusing, as it is an extra layer in between. After rereading [#1887 (comment)] and [#1887 (comment)] carefully, "we're pretty much on the same page".

One idea why roles came up in the first place.
Explicit role->permissions would prohibit "bad combined"/incoherent permissions. Only coherent permissions would be allowed and given a name. Coherent and also practical combinations may be only a few, so its convenient for humans to think of only a few roles.

As already discussed, there is no practical difference between representing "only coherent" (roles) vs. "all possible, potentially incoherent" (individual user permissions).
I was way too specific with the representation. Just wanted to react to performance converns, there are efficient solutions around, even as user (or role) ->N permissions looks like much more complicated in the first place.

Coherency issues, the only plus of thinking in roles, can be done "outside" the actual permission implementation (e.g. check_permission_coherency(user)). I'm unsure if there are pure technical incoherencies or if this is a buissness logic / specification problem.
I would only invalidate incoherent permission sets, but not require them to match known coherent sets (roles with a name).
Obviously for a user with a predefined role (taken from a (role) config file), check_permission_coherency(user) needs to pass.
Per instance or organization or even repo, a role config file would be sufficient, using common names or taken from a specific domain.

From a UX perspective, it may be a plus to restrict the selection to predefined roles. One could allow for "something else", which would require selection of each individual permission for the user. Or force the admin to update the roles config first, enusring differences in permission can easily spotted from human friendly names (maybe writeRoleConfigFile is a another good permission candidate).

I should have called user->permission_set what you called role->permissions. Talking of roles here may be confusing, as it is an extra layer in between. After rereading [https://codeberg.org/forgejo/forgejo/issues/1887#issuecomment-1530133] and [https://codeberg.org/forgejo/forgejo/issues/1887#issuecomment-1501723] carefully, "we're pretty much on the same page". One idea why roles came up in the first place. Explicit role->permissions would prohibit "bad combined"/incoherent permissions. Only coherent permissions would be allowed and given a name. Coherent and also practical combinations may be only a few, so its convenient for humans to think of only a few roles. As already discussed, there is no practical difference between representing "only coherent" (roles) vs. "all possible, potentially incoherent" (individual user permissions). I was way too specific with the representation. Just wanted to react to performance converns, there are efficient solutions around, even as user (or role) ->N permissions looks like much more complicated in the first place. Coherency issues, the only plus of thinking in roles, can be done "outside" the actual permission implementation (e.g. `check_permission_coherency(user)`). I'm unsure if there are pure technical incoherencies or if this is a buissness logic / specification problem. I would only invalidate incoherent permission sets, but not require them to match known coherent sets (roles with a name). Obviously for a user with a predefined role (taken from a (role) config file), `check_permission_coherency(user)` needs to pass. Per instance or organization or even repo, a role config file would be sufficient, using common names or taken from a specific domain. From a UX perspective, it may be a plus to restrict the selection to predefined roles. One could allow for "something else", which would require selection of each individual permission for the user. Or force the admin to update the roles config first, enusring differences in permission can easily spotted from human friendly names (maybe `writeRoleConfigFile` is a another good permission candidate).

i9e1 commented 2024年01月29日 02:24:36 +01:00

Proposal for a wording / specification / documentation, focussing on the coherency issue. This may seem a little bit to much at first, but roles / permissions are security relevant, so wording / specification, even processes should be as explicit as possible.

---

• Actions require a user to have permission to do so. [This must be implemented via a method User.haspermission(p) / User.can(p)] (very specific, but may make PR reviews easier?)

• Changing possible permissions is a breaking change. (needs discussion. Maybe adding a permission for a new feature is not breaking, but deleting a permission or merging / splitting is?)

• All permissions granted/denied for a user form the users permission set.

• All possible permission sets (= all combinations of permissions) are either coherent or incoherent.

• A permission set is incoherent if it is [specified / marked] as such (e.g. for technical reasons). All other permission sets are coherent.

• For each user, their permission set must be coherent.

• (coherent -> incoherent) Specifying a permission set as incoherent is a breacking change.

• (incoherent -> coherent) Un-specifying a permission set as incoherent is [not a breaking change but sensible / a breaking change / not allowed at all]. (needs discussion.)

• Coherent permission sets are either named or unnamed.

• A role is a named coherent permission set, roles are defined in a role configuration file.

• There is a role configuration file per [instance, organization, repo]. If a level does not have a role configuration file, it defaults to the next higher level. (pretty basic fallback for configuration)

---

flexibility / openess vs. clarity / transparency

I would not require the following, as it is more restrictive than (and duplicate of) coherent:

• For each user, their permission set must be a role.

I dont think it is necessary in the backend. From a security management perspective further restrictions to only roles may be desired.
Maybe:

• An instance may require to further restrict that for each user, their permission set must be a role.

---

also part of the specification:

• List of permissions
• List of incoherent permission sets

---

not needed to be part of a specification?

• Default role configuration file

Proposal for a wording / specification / documentation, focussing on the coherency issue. This may seem a little bit to much at first, but roles / permissions are security relevant, so wording / specification, even processes should be as explicit as possible. --- * Actions require a user to have **permission** to do so. [This must be implemented via a method `User.haspermission(p)` / `User.can(p)`] (*very specific, but may make PR reviews easier?*) * Changing possible permissions is a breaking change. (*needs discussion. Maybe adding a permission for a new feature is not breaking, but deleting a permission or merging / splitting is?*) * All permissions granted/denied for a user form the users **permission set**. * All possible permission sets (= all combinations of permissions) are either **coherent** or **incoherent**. * A permission set is incoherent if it is [specified / marked] as such (e.g. for technical reasons). All other permission sets are coherent. * For each user, their permission set must be coherent. * (coherent -> incoherent) Specifying a permission set as incoherent is a breacking change. * (incoherent -> coherent) Un-specifying a permission set as incoherent is [not a breaking change but sensible / a breaking change / not allowed at all]. (*needs discussion.*) * Coherent permission sets are either named or unnamed. * A **role** is a named coherent permission set, roles are defined in a **role configuration file**. * There is a role configuration file per [instance, organization, repo]. If a level does not have a role configuration file, it defaults to the next higher level. (*pretty basic fallback for configuration*) --- flexibility / openess vs. clarity / transparency I would *not* require the following, as it is more restrictive than (and duplicate of) coherent: * For each user, their permission set must be a role. I dont think it is necessary in the backend. From a security management perspective further restrictions to only roles may be desired. Maybe: * An instance may require to further restrict that for each user, their permission set must be a role. --- also part of the specification: * List of permissions * List of incoherent permission sets --- not needed to be part of a specification? * Default role configuration file

oliverpool commented 2024年01月29日 17:25:26 +01:00

I agree that ideally looking up if a user has permission to perform an action should not hit the database at all. However I don't think that this is hard to achieve (bitset, caching of role_id->permissions, static config...).

---

Looking at a tiny part of the current usages of models/user.User.IsAdmin, here is what I found:

• models/activities/action.go:
  • read activities of user
  • readable repositories
• models/git/lfs.go: LFS object accessibility/writability
• models/organization/org.go: org/user visibility
• models/packages/package_blob.go: access to blob
• models/perm/access/repo_permission.go: admin to repository (AccessMode & units)
• models/repo.go: deletion of deploy key
• models/user/search.go: see all users
• routers/api/v1/repo/issue_tracked_time.go: get/set tracked times of issues
• routers/api/v1/repo/migrate.go: migrate a remote repository under any user/organization
• services/repository/create.go: create repo even if the limit is already reached
• ... (and many more)

As a guess, I think that there would be 50+ different permissions (so 2^50 permission sets!!! an exhaustive definition of coherency is probably hard to achieve).

---

How should they be named/listed?
Taking inspiration of the API: {first segment of the URL}{Read/Write}{Object}:

• reposReadLFS
• userRead
• userWrite
• issueReadTrackedTimes
• issueWriteTrackedTimes
• reposIgnoreCreationLimits

Reminder: we are only talking about entity-independent permission (no mechanism to add a permission to view a given user for instance).

---

On the frontend side, I think it could be nice to have a "global switch" to enable/disable the role of the user (for itself).
So that when they are using Forgejo as a regular user, they only see what they would have access to as a regular user (preventing "mis-clicks" for instance).

I agree that ideally looking up if a user has permission to perform an action should not hit the database at all. However I don't think that this is hard to achieve (bitset, caching of role_id->permissions, static config...). --- Looking at a tiny part of the current usages of `models/user.User.IsAdmin`, here is what I found: - models/activities/action.go: * read activities of user * readable repositories - models/git/lfs.go: LFS object accessibility/writability - models/organization/org.go: org/user visibility - models/packages/package_blob.go: access to blob - models/perm/access/repo_permission.go: admin to repository (AccessMode & units) - models/repo.go: deletion of deploy key - models/user/search.go: see all users - routers/api/v1/repo/issue_tracked_time.go: get/set tracked times of issues - routers/api/v1/repo/migrate.go: migrate a remote repository under any user/organization - services/repository/create.go: create repo even if the limit is already reached - ... (and many more) As a guess, I think that there would be 50+ different permissions (so 2^50 permission sets!!! an exhaustive definition of coherency is probably hard to achieve). --- How should they be named/listed? Taking inspiration of the API: `{first segment of the URL}{Read/Write}{Object}`: - `reposReadLFS` - `userRead` - `userWrite` - `issueReadTrackedTimes` - `issueWriteTrackedTimes` - `reposIgnoreCreationLimits` Reminder: we are only talking about **entity-independent** permission (no mechanism to add a permission to view a *given* user for instance). --- On the frontend side, I think it could be nice to have a "global switch" to enable/disable the role of the user (for itself). So that when they are using Forgejo as a regular user, they only see what they would have access to as a regular user (preventing "mis-clicks" for instance).

fnetX commented 2026年05月27日 01:22:54 +02:00

Without doubt, better permission management in Forgejo is a todo. However, after scanning this issue, I conclude that there is very little information that is helpful for a future implementation. I think in this case, it would be better to first secure the capacity (e.g. via funding / sponsoring / volunteer commitment), then do the research and design for this feature without being biased by past ideas too much.

Without doubt, better permission management in Forgejo is a todo. However, after scanning this issue, I conclude that there is very little information that is helpful for a future implementation. I think in this case, it would be better to first secure the capacity (e.g. via funding / sponsoring / volunteer commitment), then do the research and design for this feature without being biased by past ideas too much.