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.

Sign up for GitHub

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

Jump to bottom

(api) Add option to enforce use of scopes (all/read, all/write, etc.) #463

Merged
merged 1 commit into from
Oct 24, 2023
Merged

(api) Add option to enforce use of scopes (all/read, all/write, etc.) #463

merged 1 commit into from
Oct 24, 2023

Conversation

Leobouloc
Copy link
Contributor

@Leobouloc Leobouloc commented Oct 12, 2023 •
edited
Loading

Purpose

The current state of Ralph allows to restrict users by authority, but does not allow 1. An admin user 2. Finer access control (read, write). This PR aims to solve this issue by implementing scopes (a field already present in user accounts) which can allow restricted access.

Proposal

The scopes proposed are a slight variation on the scopes defined by the xAPI standard:

    "statements/write",
    "statements/read/mine",
    "statements/read",
    "state/write",
    "state/read",
    "define",
    "profile/write",
    "profile/read",
    "all/read",
    "all",

NB: This PR also proposes some cleaning (renaming tests and factorizing code in tests).

TODO:

  • restrict endpoint access by scopes (RALPH_LRS_RESTRICT_BY_SCOPE)
  • deal with statements/read/mine scope (RALPH_LRS_RESTRICT_BY_AUTHORITY must be used) (specific test !)
  • test restrictions for all endpoints (GET, POST, PUT) AND for both auth methods (OIDC, and basic auth)
  • check that LRS_RESTRICT_BY_AUTHORITY==True if using scopes, and test this behavior

@Leobouloc Leobouloc changed the base branch from master to structure/refactor-statements-tests-after-unification October 12, 2023 16:33
@Leobouloc Leobouloc force-pushed the structure/refactor-statements-tests-after-unification branch from d178685 to 1783ff8 Compare October 17, 2023 07:54
@Leobouloc Leobouloc mentioned this pull request Oct 17, 2023
Closed
5 tasks
Base automatically changed from structure/refactor-statements-tests-after-unification to master October 17, 2023 08:00
@Leobouloc Leobouloc mentioned this pull request Oct 17, 2023
Closed
2 tasks
@Leobouloc Leobouloc force-pushed the feature/api/enforce-scopes-after-unification branch 3 times, most recently from 3b2e3c1 to 401db9e Compare October 17, 2023 12:32
@Leobouloc Leobouloc mentioned this pull request Oct 18, 2023
Merged
2 tasks
wilbrdt
wilbrdt approved these changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@wilbrdt wilbrdt left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉
Note that it could be nice to add or link the list of scopes defined by adlnet LRS specification, maybe in another PR?

@wilbrdt wilbrdt requested a review from jmaupetit October 19, 2023 12:05
@wilbrdt wilbrdt added this to the 4.0 milestone Oct 19, 2023
@wilbrdt wilbrdt linked an issue Oct 19, 2023 that may be closed by this pull request
Agent field is not valid openId IFI #467
Closed
2 tasks
@wilbrdt wilbrdt removed a link to an issue Oct 19, 2023
Agent field is not valid openId IFI #467
Closed
2 tasks
quitterie-lcs
quitterie-lcs reviewed Oct 20, 2023
View reviewed changes
Copy link
Contributor

@quitterie-lcs quitterie-lcs left a comment

Choose a reason for hiding this comment

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

jmaupetit
jmaupetit reviewed Oct 20, 2023
View reviewed changes
Copy link
Contributor

@jmaupetit jmaupetit left a comment

Choose a reason for hiding this comment

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

Looks great! My suggestion about using Enum for scopes can be handled in a new PR (if you are ok with this).

SergioSim
SergioSim reviewed Oct 23, 2023
View reviewed changes
Copy link
Collaborator

@SergioSim SergioSim left a comment

Choose a reason for hiding this comment

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

This looks great 👍
Have only minor questions/suggestions)

image

@Leobouloc Leobouloc force-pushed the feature/api/enforce-scopes-after-unification branch from 8c1968e to 34c8413 Compare October 24, 2023 10:12
@Leobouloc Leobouloc mentioned this pull request Oct 24, 2023
Open
@Leobouloc
Copy link
Contributor Author

LGTM! 🎉 Note that it could be nice to add or link the list of scopes defined by adlnet LRS specification, maybe in another PR?

Added here: https://github.com/openfun/ralph/blob/bd6fcb615e3a5cba536505f8a6448c234696f369/docs/api.md#scopes !

@Leobouloc
✨(api) add option to enforce scopes
df62058 
The current state of Ralph allows to restrict users by authority, but does not
allow a/An admin user b/Finer access control (read, write). This PR aims to
solve this issue by implementing `RESTRICT_BY_SCOPES` (`scopes` field is
already present in user accounts) which restricts access when enabled.
@Leobouloc Leobouloc force-pushed the feature/api/enforce-scopes-after-unification branch from bd6fcb6 to df62058 Compare October 24, 2023 15:16
@Leobouloc Leobouloc enabled auto-merge (rebase) October 24, 2023 15:19
@Leobouloc Leobouloc merged commit 04c636f into master Oct 24, 2023
@Leobouloc Leobouloc deleted the feature/api/enforce-scopes-after-unification branch October 24, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmaupetit jmaupetit jmaupetit left review comments

@SergioSim SergioSim SergioSim left review comments

@quitterie-lcs quitterie-lcs quitterie-lcs left review comments

@wilbrdt wilbrdt wilbrdt approved these changes

Assignees

@Leobouloc Leobouloc

Labels
feature needs review
Projects
Ralph roadmap
Status: Done
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Leobouloc @jmaupetit @wilbrdt @SergioSim @quitterie-lcs