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

BC-5195 - poc permission context #4555

Closed
wants to merge 36 commits into from
Closed

Conversation

EzzatOmar
Copy link
Contributor

Description

This Proof of Concept (PoC) addresses an existing constraint in our role-based permission system.

The problem:

Consider a scenario where a user with the role "STUDENT" wishes to modify a card on the board. Simply identifying the user's role and associated permissions is insufficient to determine whether they have the authorization to modify the card.

For instance, let's say we have two courses, each with their own boards. A student from class A should not have the ability to edit a card on the board belonging to course B. This implies that a generic "STUDENT" role is inadequate. Instead, we would need distinct roles like "STUDENT OF COURSE A" and "STUDENT OF COURSE B".

Proposed solution:

This PR proposes the introduction of a permission_context that includes a list of users along with their permissions. It also supports nested contexts, simplifying permission computation by adjusting a base context. For example, the permissions for a card can be set to mirror those of the board it belongs to.

The PoC is still a work in progress. User grouping (via roles, teams, etc.) has not been implemented yet, but should be straightforward to add once the overall PoC receives positive feedback.

Currently, the permission_context cannot be added or edited. For testing on localhost, this needs to be done manually.

Here's an example to add to the permission-context collection to authorize a board:

{
    "_id" : ObjectId("65528f7007eec42942904435"),
    "name" : "first board ctx",
    "contextReference" : ObjectId("6551131b4d8599f33d461cde"),
    "parentContext" : null,
    "userDelta" : {
        "0000d231816abba584714c9e" : {
            "includedPermissions" : [
                "BOARD_READ"
            ],
            "excludedPermissions" : [

            ]
        }
    }
}

Links to Tickets or other pull requests

https://ticketsystem.dbildungscloud.de/browse/BC-5195

Changes

A new collection permission-context has been added. Each document represents a context and includes a list of user IDs with their permissions and a pointer to a document signifying the object to be authorized. Duplicate contexts are prohibited (currently enforced by a unique constraint).

The introduced PermissionContextService offers the functionality to resolve permissions. All other entities are unaware of the permissionContext, maintaining a unidirectional flow, similar to an addon. The UC layer only needs to use the PermissionContextService.

image

Datasecurity

Deployment

New Repos, NPM pakages or vendor scripts

Approval for review

  • DEV: If api was changed - generate-client:server was executed in vue frontend and changes were tested and put in a PR with the same branch name.
  • QA: In addition to review, the code has been manually tested (if manual testing is possible)
  • All points were discussed with the ticket creator, support-team or product owner. The code upholds all quality guidelines from the PR-template.

Notice: Please remove the WIP label if the PR is ready to review, otherwise nobody will review it.

@EzzatOmar EzzatOmar added the WIP This feature branch is in progress, do not merge into master. label Nov 13, 2023
@EzzatOmar EzzatOmar requested review from virgilchiriac and Metauriel and removed request for virgilchiriac November 13, 2023 22:28
@EzzatOmar
Copy link
Contributor Author

THIS PR MUST NOT MERGED INTO MAIN

Permission.BOARD_COLUMN_CREATE,
Permission.BOARD_COLUMN_MOVE,
Permission.BOARD_COLUMN_DELETE,
Permission.BOARD_COLUMN_UPDATE_TITLE,
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if update title needs a permission by itself
edit column should cover that (also move / delete column)

export class PermissionContextEntity extends BaseEntityWithTimestamps {
@Property()
@Unique()
contextReference: ObjectId;
Copy link
Contributor

Choose a reason for hiding this comment

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

should use EntityId instead of ObjectId

BOARD_ELEMENT_UPDATE = 'BOARD_ELEMENT_UPDATE',
BOARD_ELEMENT_DELETE = 'BOARD_ELEMENT_DELETE',
BOARD_ELEMENT_MOVE = 'BOARD_ELEMENT_MOVE',
BOARD_ELEMENT_CAN_SUBMIT = 'BOARD_SUBMISSION_CAN_SUBMIT',
Copy link
Contributor

Choose a reason for hiding this comment

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

One idea would be to have only a limitted number of permitted actions (normally CRUD - but right now we tend to use only read and write only in the authorization / rules )
The "string" permissions are old, and should be avoided, as long as you have permission on the entity


@ManyToOne(() => PermissionContextEntity, { wrappedReference: true, fieldName: 'parentContext', nullable: true })
@Index()
_parentContext: Reference<PermissionContextEntity> | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

why the notation with _ ?


return permissionMatrix;
}
}
Copy link
Contributor

@virgilchiriac virgilchiriac Nov 21, 2023

Choose a reason for hiding this comment

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

where would the user role come into play?

We might have something like implicit perrmissions frrom the role, and then, explicit permission here?

actor (group / role / user) entity (collection or document) action allowed
student column-node read true
student column-node write false
teacher column-node write true
...
userId column-node-id write true
userId card-node-id read false

also, the relation with parent will be difficult to manage... whenever the parent is updated

@EzzatOmar EzzatOmar closed this Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge do-not-merge WIP This feature branch is in progress, do not merge into master.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants