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

Add role management endpoints #8984

Merged
merged 5 commits into from
Oct 1, 2024
Merged

Add role management endpoints #8984

merged 5 commits into from
Oct 1, 2024

Conversation

leovalais
Copy link
Contributor

@leovalais leovalais commented Sep 24, 2024

Tip

Review each commit individually.

Refs #8743

This PR lacks the following elements:

  • Unit tests
    • Authorizer logic
    • PgAuthDriver
    • Endpoints
  • Documentation

However I suggest we merge it like this if its content is good enough to allow @kmer2016 to work on the frontend part as soon as possible. Consequently, to also sum up the comments of this PR, we'll need to:

  1. create a CLI command to grant builtin roles to a subject: RBAC: CLI interface #8744
  2. remove application roles which are more than superfluous until we have a proper admin panel: Add role management endpoints #8984 (comment)
  3. work on tests and documentation
  4. improve DbConnection API: Add role management endpoints #8984 (comment)

@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 49.09091% with 140 lines in your changes missing coverage. Please review.

Project coverage is 37.14%. Comparing base (795c30a) to head (ce257e6).
Report is 26 commits behind head on dev.

Files with missing lines Patch % Lines
editoast/src/views/authz.rs 32.08% 91 Missing ⚠️
editoast/editoast_authz/src/authorizer.rs 0.00% 26 Missing ⚠️
front/src/common/api/generatedEditoastApi.ts 91.01% 8 Missing ⚠️
editoast/editoast_authz/src/roles.rs 0.00% 7 Missing ⚠️
editoast/src/models/auth.rs 50.00% 7 Missing ⚠️
editoast/src/views/mod.rs 50.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #8984      +/-   ##
============================================
+ Coverage     37.11%   37.14%   +0.03%     
  Complexity     2241     2241              
============================================
  Files          1262     1263       +1     
  Lines        115834   116059     +225     
  Branches       3277     3277              
============================================
+ Hits          42991    43110     +119     
- Misses        70889    70995     +106     
  Partials       1954     1954              
Flag Coverage Δ
core 74.79% <ø> (ø)
editoast 72.16% <29.03%> (-0.22%) ⬇️
front 15.29% <91.01%> (+0.10%) ⬆️
gateway 2.22% <ø> (ø)
osrdyne 2.57% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 86.46% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

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

I hope the entire PR will be squashed because

#[cfg(test)]
mod test_app;

is removed in e08d2f4 and only comes back 3 commits later in 6c32f1a. Similar inconsistencies happen when utoipa annotations are added/modified but openapi.yaml is updated in a subsequent commit.

editoast/editoast_authz/src/authorizer.rs Outdated Show resolved Hide resolved
editoast/editoast_authz/src/authorizer.rs Outdated Show resolved Hide resolved
editoast/editoast_authz/src/authorizer.rs Outdated Show resolved Hide resolved
editoast/openapi.yaml Outdated Show resolved Hide resolved
editoast/openapi.yaml Show resolved Hide resolved
editoast/src/models/auth.rs Outdated Show resolved Hide resolved
editoast/src/models/auth.rs Outdated Show resolved Hide resolved
editoast/src/views/authz.rs Outdated Show resolved Hide resolved
front/public/locales/fr/errors.json Outdated Show resolved Hide resolved
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

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

Great PR.

I think it's going to be a pain not to have a “role config” in this repository but in the deployment repository. It's great that it can be overridden, but knowing that you're likely to want to update it regularly (until the whole thing has stabilized and settled down), it's error prone.

editoast/src/views/authz.rs Outdated Show resolved Hide resolved
@leovalais
Copy link
Contributor Author

leovalais commented Sep 25, 2024

I hope the entire PR will be squashed

It will. I just split it to help with the review. I may keep a few commits (especially for the stdcm env part), but I'll split it right this time 😁

I think it's going to be a pain not to have a “role config” in this repository but in the deployment repository. It's great that it can be overridden, but knowing that you're likely to want to update it regularly (until the whole thing has stabilized and settled down), it's error prone.

Agreed. I am becoming increasingly convinced that we don't need application roles after all. (Or at least until we have a proper administration panel.) Maybe it wouldn't be so bad to remove them since we are the ones who will grant roles anyway.

@flomonster
Copy link
Contributor

Agreed. I am becoming increasingly convinced that we don't need application roles after all. (Or at least until we have a proper administration panel.) Maybe it wouldn't be so bad to remove them since we are the ones who will grant roles anyway.

Another argument is that with the current implementation, changing the scope of an application role won't change user roles. (Except if we do a proper migration). It reduces the usefulness of this object in my opinion.

@leovalais leovalais force-pushed the roles-rest-api branch 2 times, most recently from 62486a1 to 3c6a80d Compare September 30, 2024 12:58
Since a new Authorizer is created for each request, it doesn't needs to
be refcounted. However it still needs to be Clone otherwise axum rejects
the type. Removing the Arc allows easier mutability.

Signed-off-by: Leo Valais <leo.valais97@gmail.com>
Builtin roles are supposed to be atomic. Having to resolve both builtin
roles and application roles makes all algorithms more cumbersome to
write and induces some questions about whether or not we write implied
builtin roles in the DB.

Signed-off-by: Leo Valais <leo.valais97@gmail.com>
Signed-off-by: Leo Valais <leo.valais97@gmail.com>
Signed-off-by: Leo Valais <leo.valais97@gmail.com>
Signed-off-by: Leo Valais <leo.valais97@gmail.com>
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

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

Tested and not working. No user is created when using the application.

Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

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

LGTM (tested).
Before using it in prod we need:

  • Add tests
  • Remove application role
  • Add CLI

Copy link
Contributor

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

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

Lgtm (not tested)

@leovalais leovalais added this pull request to the merge queue Oct 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 1, 2024
@leovalais leovalais added this pull request to the merge queue Oct 1, 2024
Merged via the queue into dev with commit 91021c9 Oct 1, 2024
23 checks passed
@leovalais leovalais deleted the roles-rest-api branch October 1, 2024 16:57
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.

5 participants