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

(api) Add option to enforce use of scopes (all/read, all/write, etc.) [after #448] #441

Closed

Conversation

Leobouloc
Copy link
Contributor

@Leobouloc Leobouloc commented Sep 20, 2023

Merge after unification & test refactoring [#448 ]

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
  • deal with statements/read/mine scope (ENFORCE_AUTHORITY must be used) (specific test !)
  • test restrictions for all endpoints (GET, POST, PUT) AND for both auth methods (OIDC, and basic auth)
  • (side quest) rename tests removing extra "statement" (i.e. test_api_statements_get_statements -> test_api_statements_get)
  • (side quest) create a mock_statement helper to simplify post and put tests

@Leobouloc Leobouloc added the WIP label Sep 20, 2023
@Leobouloc Leobouloc changed the title Enforce scopes (api) Add option to enforce use of scopes (all/read, all/write, etc.) Sep 20, 2023
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.

Alt text

Comment on lines +30 to +37
"statements/read": {"statements/read/mine", "statements/read"},
"all/read": {
"statements/read/mine",
"statements/read",
"state/read",
"profile/read",
"all/read",
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be possible to define them with a wildcard? E.g. statements/read/* and */read/*?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefered this explicit syntax as it seemed clearer when reading the code, but I can change this.

Anyone else has an opinion ?

@Leobouloc Leobouloc mentioned this pull request Oct 4, 2023
16 tasks
@Leobouloc Leobouloc changed the base branch from master to structure/refactor-statements-tests October 4, 2023 16:50
@Leobouloc Leobouloc changed the title (api) Add option to enforce use of scopes (all/read, all/write, etc.) (api) Add option to enforce use of scopes (all/read, all/write, etc.) [after #448] Oct 5, 2023
@Leobouloc Leobouloc deleted the branch structure/refactor-statements-tests October 12, 2023 12:40
@Leobouloc Leobouloc closed this Oct 12, 2023
@Leobouloc
Copy link
Contributor Author

Replaced by #463

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants