OEP-66: Authorization Best Practices #520

hsinkoff · 2023-09-12T15:22:03Z

No description provided.

feanil

@hsinkoff I left a couple of small comments but this generally looks good. Do you want to take a pass through this before we push this out of draft and open it up for general review?

feanil · 2023-09-20T14:57:25Z

oeps/best-practices/oep-0066-bp-authorization.rst

+     - `OEP-04`_, `OEP-42`_
+
+.. _OEP-04: https://open-edx-proposals.readthedocs.io/en/latest/oeps/oep-0004.html
+.. _OEP-42: https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0042-bp-authentication.html


Suggested change

- `OEP-04`_, `OEP-42`_

.. _OEP-04: https://open-edx-proposals.readthedocs.io/en/latest/oeps/oep-0004.html

.. _OEP-42: https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0042-bp-authentication.html

-

* :doc:`/architectural-decisions/oep-0004-arch-oauth-scopes`

* :doc:`/best-practices/oep-0042-bp-authentication`

When in the same repo, we should link to the local version of the document so that local builds don't accidentally point to the deployed docs.

feanil · 2023-09-20T14:59:56Z

oeps/best-practices/oep-0066-bp-authorization.rst

+Implicit roles grant users permissions, but are not specifically assigned 
+to a user.
+
+Systems/Protocols Overview


I think it would be useful to move the Best Practice section above this overview since that is meant to replace these other ways of managing permissions. I think it would also be helpful to be more clear that these tactics should be avoided unless there are very good reasons not to do this.

hsinkoff · 2023-09-20T15:04:17Z

@feanil Yes, give me 24 hours to make the updates first.

oeps/best-practices/oep-0066-bp-authorization.rst

oeps/best-practices/oep-0009-bp-permissions.rst

oeps/best-practices/oep-0066/Open_edX_Authorization_Systems_Table.rst

feanil · 2023-10-17T14:56:31Z

One more day for general review, then I'll do a final review to figure out if there are any changes needed before we can merge this.

sarina

This looks really great - thanks for the hard work on this, everyone. I added just a couple questions and some style notes (particularly, "Open edX" is an adjective per our trademark guidelines).

sarina · 2023-10-18T16:05:22Z

oeps/best-practices/oep-0066-bp-authorization.rst

+
+There are a variety of manners in which authorization is handled within the Open edX ecosystem. 
+The goal of OEP-66 is to provide best practices that should be used with all the 
+systems/protocols and outline the currently in use systems/protocols.


Suggested change

systems/protocols and outline the currently in use systems/protocols.

systems/protocols and outline the systems/protocols currently in use.

I had to reread this phrasing a few times to understand what was trying to be said

sarina · 2023-10-18T16:05:53Z

oeps/best-practices/oep-0066-bp-authorization.rst

+   * - Authors
+     - Hilary Sinkoff (hsinkoff@2u.com), Jeremy Bowman (jbowman@edx.org)   
+   * - Arbiter
+     - Feanil Patel


Suggested change

- Feanil Patel

- Feanil Patel <feanil@axim.org>

sarina · 2023-10-18T16:06:10Z

oeps/best-practices/oep-0066-bp-authorization.rst

+Best Practices Motivation
+--------------------------
+To date, the implementation and verification of permissions have been somewhat
+conflated in the edX codebase.  When a user attempts an action which is not


Suggested change

conflated in the edX codebase. When a user attempts an action which is not

conflated in the Open edX codebase. When a user attempts an action which is not

sarina · 2023-10-18T16:07:42Z

oeps/best-practices/oep-0066-bp-authorization.rst

+unified documentation on the systems/protocols. This OEP aims to compile existing 
+knowledge and documentation into a central document that will give an overview of each 
+system/protocol. The aim is not to be the only source of information for each system/protocol, 
+but rather a starting point when learning about authorization within Open edX.


Suggested change

but rather a starting point when learning about authorization within Open edX.

but rather a starting point when learning about authorization within the Open edX codebase.

"Open edX" should be used as an adjective

sarina · 2023-10-18T16:08:36Z

oeps/best-practices/oep-0066-bp-authorization.rst

+  it can be difficult to avoid accidentally changing other permissions with
+  a similar implementation.
+
+In Use System/Protocol Motivation


This title confuses me and I don't know what even to suggest. Is it something like, "Currently Used Systems/Protocols"?

sarina · 2023-10-18T16:14:44Z

oeps/best-practices/oep-0066-bp-authorization.rst

+Open edX Authorization Systems Diagram
+
+.. image:: oep-0066/Open_edX_Authorization.png
+   :alt: A diagram that shows the different systems/protocols used in Open edX to control authorization. The information in the diagram is also in the Open edX Authorization Systems Table (linked to in this document).


Suggested change

:alt: A diagram that shows the different systems/protocols used in Open edX to control authorization. The information in the diagram is also in the Open edX Authorization Systems Table (linked to in this document).

:alt: A diagram that shows the different systems/protocols used in the Open edX codebase to control authorization. The information in the diagram is also in the Open edX Authorization Systems Table (linked to in this document).

sarina · 2023-10-18T16:15:25Z

oeps/best-practices/oep-0066-bp-authorization.rst

+
+   oep-0066/Open_edX_Authorization_Systems_Table.rst
+
+django Admin (auth_permission)


is there a reason django is lowercase in this section but uppercase previously?

sarina · 2023-10-18T16:15:58Z

oeps/best-practices/oep-0066-bp-authorization.rst

+In this way, it can grant permissions to a user that they will not be able to use.
+
+auth_permission users and groups are assigned through the django Admin Dashboard. Each 
+service can have its own django Admin Dashboard. In Open edX, the LMS django Admin Dashboard 


Suggested change

service can have its own django Admin Dashboard. In Open edX, the LMS django Admin Dashboard

service can have its own django Admin Dashboard. In the Open edX software, the LMS django Admin Dashboard

sarina · 2023-10-18T16:17:41Z

oeps/best-practices/oep-0066-bp-authorization.rst

+on data that is not role assignment data. 
+
+Implicit roles grant users permissions, but are not specifically assigned 
+to a user.


Implicit roles don't seem... good? I understand this is just a definition section, but this definition leaves me scratching my head about the "why" this would possibly exist in a system

sarina · 2023-10-18T16:20:30Z

oeps/best-practices/oep-0066/Open_edX_Authorization_Systems_Table.rst

+          * auth_user_groups table with group_id, id, user_id, _sdc_deleted_at fields
+          * auth_user_user_permissions table with id, permission_id, user_id fields 
+
+edX rbac


since this is a code package should we call it edx-rbac (with dash, no lower case) here and within the following table?

robrap

@hsinkoff: Apologies that I forgot about this and can't immediately get to a detailed review. I understand that that means this will land without that, which is completely fine. Another general comment is that there might be some definitions or details that are noted in https://openedx.atlassian.net/wiki/spaces/AC/pages/935919751/Authorization that might be helpful. For example, what is a "system-wide role" and what are examples? What is a super user, and global staff, etc.? Something to consider. We discussed this doc in the past, so maybe you've already captured everything you wish to.

oeps/best-practices/oep-0066-bp-authorization.rst

robrap

Thank you for bringing in some updates from my examples from https://openedx.atlassian.net/wiki/spaces/AC/pages/935919751/Authorization. Rather than me going through each point, I'd like to ask my question in a different way...

Once this OEP lands, would you feel comfortable archiving the above Confluence doc with a note that reads that its contents are now represented in OEP-66. If the answer is yes, great. If the answer is no, please consider moving in whatever gaps you find.

Also, maybe you can respond with a comment as well, if needed. For example, do you think the diagrams in that document are redundant, or unclear, or misleading, or unhelpful?

I'm hopeful that this OEP can replace older Authorization related documentation, which is why I ask. This is what I did with legacy Authentication related documentation when creating the AuthN OEP.

Thank you. Also, I don't have really strong opinions because my head is not in that space. @iloveagent57 made edits to that page, so maybe he has stronger opinions?

oeps/best-practices/oep-0066-bp-authorization.rst

hsinkoff · 2023-10-20T16:08:45Z

One additional diagram, along with text describing the diagram, and key take-aways from it have been added to the OEP. With this addition, I think the OEP will allow for archiving https://openedx.atlassian.net/wiki/spaces/AC/pages/935919751/Authorization

robrap · 2023-10-20T19:37:35Z

oeps/best-practices/oep-0066-bp-authorization.rst

 * Global Staff - propogated in JWTs as the "administrator" field

+Example AuthZ User Access Flows
+-------------------------------
+.. image:: oep-0066/Open_edX_Authorization_User_Flows.png


Can you provide a link to the diagram source as well?

.. Something like this might add a hidden note in the doc.

The diagram was created specifically for this OEP.

It is based on one in https://openedx.atlassian.net/wiki/spaces/AC/pages/935919751/Authorization, but with a few modifications aimed at highlighting certain use cases. The changes are significant enough that I don't think we'd want to link to that diagram as the source.

Right. I meant the source of the new png. How was it created, and what happens if someone wants to update it? (Same might be true of other diagrams.)

From OEP-19:

Exported Diagrams. In order to keep diagrams updated across time, they will need to be exported from whichever tool they were created in, with the exported version maintained in GitHub.

draw.io diagrams will be exported and imported as XML files.

Lucidchart diagrams will be exported and imported as Visio (VDX) files.

I've re-worked the diagrams in order to ensure they are editable in the future. I've also separated the data tables by diagram. Please take a look when you have a chance.

…e diagrams

feanil

Looks like all comments have been addressed and I see no major issues. I'm marking this OEP as accepted and will merge it shortly.

hsinkoff requested a review from feanil September 12, 2023 15:22

jmbowman mentioned this pull request Sep 18, 2023

Refactor authorization checks edx/edx-arch-experiments#451

Open

feanil reviewed Sep 20, 2023

View reviewed changes

hsinkoff marked this pull request as ready for review September 27, 2023 14:21

mariajgrimaldi reviewed Sep 29, 2023

View reviewed changes

oeps/best-practices/oep-0066-bp-authorization.rst Outdated Show resolved Hide resolved

mariajgrimaldi reviewed Sep 29, 2023

View reviewed changes

oeps/best-practices/oep-0066-bp-authorization.rst Outdated Show resolved Hide resolved

robrap reviewed Oct 2, 2023

View reviewed changes

oeps/best-practices/oep-0009-bp-permissions.rst Outdated Show resolved Hide resolved

robrap reviewed Oct 3, 2023

View reviewed changes

oeps/best-practices/oep-0066/Open_edX_Authorization_Systems_Table.rst Outdated Show resolved Hide resolved

sarina reviewed Oct 18, 2023

View reviewed changes

robrap reviewed Oct 18, 2023

View reviewed changes

robrap reviewed Oct 19, 2023

View reviewed changes

oeps/best-practices/oep-0066-bp-authorization.rst Outdated Show resolved Hide resolved

robrap reviewed Oct 19, 2023

View reviewed changes

oeps/best-practices/oep-0066-bp-authorization.rst Outdated Show resolved Hide resolved

oeps/best-practices/oep-0066-bp-authorization.rst Outdated Show resolved Hide resolved

robrap reviewed Oct 20, 2023

View reviewed changes

hsinkoff added 14 commits October 26, 2023 11:34

docs: oep-66 authorization

e9f3323

docs: update and add information from oep-0009

9bf01ee

docs: add diagram of systems

3c1a165

docs: add table for improved accessibility

069a62f

docs: update review period

16ca8c7

docs: add PR link

18ba106

docs: re-order content, update internal links

0d9cfb6

docs: update link to django 3.2 link

ea982d5

docs: add oep-66 motivation, update oep-9 status

d5d6e6c

docs: update last modified and changelog information

6a6e8a5

docs: update table format

fa4bec5

feat: updates based on code reviews (wordchoice, syntax)

0d10a8e

feat: add super-user and system-wide roles to docs

8a50455

feat: typo updates in OEP-66

5615a80

hsinkoff and others added 5 commits October 26, 2023 11:34

feat: add user flow diagram and description

ec9224c

feat: provide diagrams and data tables in editable format, re-organiz…

0254d63

…e diagrams

feat: update diagram colors to improve readability

ce2f79b

docs: update edit history

cebd7c2

docs: Move OEP-9 to the archived section.

f2fb19c

feanil force-pushed the hs/authorization_best_practice branch from d853396 to 8d1333f Compare October 26, 2023 15:58

docs: Add a warning and fix broken links in the archived doc.

200f65a

feanil force-pushed the hs/authorization_best_practice branch from 8d1333f to 200f65a Compare October 26, 2023 18:04

docs: Mark the new OEP as accepted.

27c3ceb

feanil approved these changes Oct 26, 2023

View reviewed changes

feanil merged commit 9a74ad7 into master Oct 26, 2023
2 checks passed

feanil deleted the hs/authorization_best_practice branch October 26, 2023 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OEP-66: Authorization Best Practices #520

OEP-66: Authorization Best Practices #520

hsinkoff commented Sep 12, 2023

feanil left a comment

feanil Sep 20, 2023

feanil Sep 20, 2023

hsinkoff commented Sep 20, 2023

feanil commented Oct 17, 2023

sarina left a comment

sarina Oct 18, 2023

sarina Oct 18, 2023

sarina Oct 18, 2023

sarina Oct 18, 2023

sarina Oct 18, 2023

sarina Oct 18, 2023

sarina Oct 18, 2023

sarina Oct 18, 2023

sarina Oct 18, 2023

sarina Oct 18, 2023

robrap left a comment

robrap left a comment

hsinkoff commented Oct 20, 2023 •

edited

Loading

robrap Oct 20, 2023

hsinkoff Oct 20, 2023

robrap Oct 20, 2023

hsinkoff Oct 23, 2023

feanil left a comment

	systems/protocols and outline the currently in use systems/protocols.
	systems/protocols and outline the systems/protocols currently in use.

	conflated in the edX codebase. When a user attempts an action which is not
	conflated in the Open edX codebase. When a user attempts an action which is not

	but rather a starting point when learning about authorization within Open edX.
	but rather a starting point when learning about authorization within the Open edX codebase.

	:alt: A diagram that shows the different systems/protocols used in Open edX to control authorization. The information in the diagram is also in the Open edX Authorization Systems Table (linked to in this document).
	:alt: A diagram that shows the different systems/protocols used in the Open edX codebase to control authorization. The information in the diagram is also in the Open edX Authorization Systems Table (linked to in this document).


		oep-0066/Open_edX_Authorization_Systems_Table.rst

		django Admin (auth_permission)

	service can have its own django Admin Dashboard. In Open edX, the LMS django Admin Dashboard
	service can have its own django Admin Dashboard. In the Open edX software, the LMS django Admin Dashboard

OEP-66: Authorization Best Practices #520

OEP-66: Authorization Best Practices #520

Conversation

hsinkoff commented Sep 12, 2023

feanil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsinkoff commented Sep 20, 2023

feanil commented Oct 17, 2023

sarina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robrap left a comment

Choose a reason for hiding this comment

robrap left a comment

Choose a reason for hiding this comment

hsinkoff commented Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feanil left a comment

Choose a reason for hiding this comment

hsinkoff commented Oct 20, 2023 •

edited

Loading