Skip to content
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

Addition of contributor role to metamist permissions #703

Closed
wants to merge 16 commits into from

Conversation

dancoates
Copy link
Contributor

@dancoates dancoates commented Mar 14, 2024

Context

Metamist currently has a fairly simple permission system. Each project defines a read group and a write group, these two groups define a list of group members. The members of the read group will have readonly access to the project, and the members of the write group have full unrestricted write access to the project.

Write actions within metamist are primarily performed programmatically via the API, but with some upcoming new features there will soon be the need to allow human users to perform a limited set of write actions. Some examples of these new features are:

  • Custom cohorts, where we will want to allow users to create cohorts
  • Attachments, where we will want to allow users to attach comments or other attachment types to various entities within metamist.

We do not want to grant full write access to allow users to perform these actions, so it is necessary to create a more nuanced role based permissions system that allows limited access to certain write operations but restricts access to anything destructive.

Proposed Solution

We will keep the existing read and write permissions, as they are used extensively, and introduce a new contribute role that has limited write access. To achieve this we can introduce the concept of group roles, and migrate the existing read and write permissions to corresponding read and write roles, while also adding the new contribute role.

Types of roles

We will initially have 3 roles

  • read
  • contribute
  • write

With just these three roles they could be modeled as a hierarchical role system where each role inherits permissions from the last, ie. contribute inherits all permissions that read has. This model could cause problems in the future if we wanted to introduce more roles that don't directly fit into the hierarchical system. For example an editor role that allows for some write actions, but not the same ones as contribute.

For this reason in this PR they are modeled as mutually exclusive roles and it is necessary to specify each role that is allowed to perform any given action. ie. if you want members of the write group to be able to perform read actions, then the write role will need to be listed as an allowed role.

Database setup

The addition of roles requires a fairly minimal change to the database, just the addition of one new join table project_groups. The contains the join between a project and a group and the role that the group has within a project. It should look like:

CREATE TABLE `project_groups` (
  `project_id` int(11) NOT NULL,
  `group_id` int(11) NOT NULL,
  `role` enum('read','write','contribute') NOT NULL,
  `audit_log_id` int(11) DEFAULT NULL,
  KEY `FK_PROJECT_GROUPS_PROJECT_ID` (`project_id`),
  KEY `FK_PROJECT_GROUPS_GROUP_ID` (`group_id`),
  KEY `FK_PROJECT_GROUPS_CHANGELOG_ID` (`audit_log_id`),
  CONSTRAINT `FK_PROJECT_GROUPS_CHANGELOG_ID` FOREIGN KEY (`audit_log_id`) REFERENCES `audit_log` (`id`),
  CONSTRAINT `FK_PROJECT_GROUPS_GROUP_ID` FOREIGN KEY (`group_id`) REFERENCES `group` (`id`),
  CONSTRAINT `FK_PROJECT_GROUPS_PROJECT_ID` FOREIGN KEY (`project_id`) REFERENCES `project` (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;

So best to limit the destructive updates in the migration and do them manually
This way they are accessible pretty much everywhere, but are only calculated once. The permission checks themselves are now synchronous and should be really fast, so no need for avoiding checking project ids
Also some QOL fixes for graphql types so that the context is now properly typed
Rather than having these roles separately - incorportate them into
project level roles where it makes sense. That way the same permission
constructs can be used for checking admin roles rather than having to
have separate ones.
even when running locally, it is better to lean on the actual access
controls rather than allowing all access. This way we can catch issues
with permission checks during development
to avoid listing absolutely every project for users with admin roles
@dancoates dancoates closed this Jun 18, 2024
@dancoates dancoates deleted the contributor-role branch October 28, 2024 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant