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

OEP-66 RBAC (Role Based Access Control) - DRAFT #510

Closed
wants to merge 3 commits into from

Conversation

hsinkoff
Copy link
Member

No description provided.

@hsinkoff hsinkoff requested a review from feanil August 17, 2023 18:10
@hsinkoff hsinkoff requested a review from robrap August 17, 2023 18:12
Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Some initial thoughts. Thank you @hsinkoff.

There are currently two distinct systems for adding course level roles that are comingled in the UI.
The database tables involved in the first system (course access roles) is the student_courseaccessrole table
and the database tables involved in the second system (discussion roles) are the django_comment_client_role table and associated tables.
Additionally, it is possible to grant a user access to a course using the Django Admin auth_permission table and associated tables.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this newline work for you? I was getting confused trying to see how this was related to the two distinct systems.

Suggested change
Additionally, it is possible to grant a user access to a course using the Django Admin auth_permission table and associated tables.
Additionally, it is possible to grant a user access to a course using the Django Admin auth_permission table and associated tables.

Comment on lines +38 to +39
The database tables involved in the first system (course access roles) is the student_courseaccessrole table
and the database tables involved in the second system (discussion roles) are the django_comment_client_role table and associated tables.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these both in edx-platform? Are there other parts of this that are already used in multiple services? Maybe you could add some notes around this.

You do have the following below:

The current systems require developer work across multiple repos, MFEs, and IDAs ...

It might be helpful to clarify the parts that are cross-service (IDA) to make it clear this is an OEP, and not just an edx-platform level ADR. Somehow the "two distinct systems" is what sticks out the most when I'm reading this, and these seem to be contained in one service (I think).

See the "Decision" section for a similar comment.


We will create a new system that utilizes permissions assigned to roles
which are assigned to users to grant access to different portions of the system.
This system will replace the two existing systems, course access roles and discussion roles.
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, this decision seem focused on edx-platform systems (I think). Can you help clarify why this shouldn't just be an edx-platform ADR.


Once completed this will remove one level of complexity from the overall authorization picture
by decreasing the systems involved by one, however during the interim it will add complexity by increasing
the number of involved systems and requiring all access checks to check for a role and a permission.
Copy link
Contributor

Choose a reason for hiding this comment

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

The term "system" is possibly confusing me. Will this check be within a service, or cross-service? Depending on the answer, we can discuss potential language. "System" made me think cross-service, but I don't know that that is the case.

References
**********

.. image:: oep-0066/rbac_overview.png
Copy link
Contributor

Choose a reason for hiding this comment

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

This image contains much more detail that is not in this document. Is there another doc that contains this information? Do you want a sub-document or other sections? For accessibility purposes (which would probably help most readers), having this information in doc form would be useful. We could discuss options.

Also, this image contains so much. Even just breaking it into 2 images, rather than one that is so wide, would help give each more room.

- TBD

*******
Context
Copy link
Contributor

Choose a reason for hiding this comment

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

I was surprised that there was no mention of the edx-rbac library. Is that being described below, but in other language? Is that not being discussed at all? It feels like an RBAC OEP should at least acknowledge this library and explain why it is or is not part of this OEP.

@hsinkoff
Copy link
Member Author

After speaking with @robrap and @feanil it has been decided that a best practice OEP for Authorization is needed and will include information on course level roles, but that an architecture OEP for this change is not needed. This information will be included in an ADR in the edx-platform repo and then documented as an authorization option in the best practice OEP.

OEP-66 will be transformed into the Authorization best practice OEP. A new PR will be opened once a first draft of the authorization best practice OEP is ready.

@hsinkoff hsinkoff closed this Aug 22, 2023
@hsinkoff hsinkoff deleted the hs/course_roles_architecture branch August 22, 2023 19:15
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.

2 participants