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

Role-based authorization system #7981

Merged
merged 1 commit into from
Aug 1, 2024
Merged

Role-based authorization system #7981

merged 1 commit into from
Aug 1, 2024

Conversation

leovalais
Copy link
Contributor

@leovalais leovalais commented Jul 8, 2024

Important

Tracking issue: #8171

Implementation plan

  • diesel migration
  • Builtin role definition
  • Role config parsing
  • struct Authorizer
  • User registration using request headers
  • Role fetching from db
  • Role checking
  • Test using one endpoint (/health)
  • Role CRD operations
  • Rewrite the PgAuthDriver using diesel::dsl
  • Unit tests
    • Role config parsing (in progress)
    • Role checking function
    • PgAuthDriver

Finitions

  • Review naming
  • mv modelsv2/authn.rs modelsv2/auth.rs
  • Documentation of the definitive parts of the API
  • Try to reduce (within the scope of the PR ofc) the truckload of new TODO: comments

@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2024

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

Codecov Report

Attention: Patch coverage is 79.20605% with 110 lines in your changes missing coverage. Please review.

Project coverage is 27.41%. Comparing base (b003629) to head (1f12f4e).
Report is 52 commits behind head on dev.

Files Patch % Lines
editoast/editoast_authz/src/authorizer.rs 80.76% 35 Missing ⚠️
editoast/editoast_authz/src/roles.rs 82.35% 24 Missing ⚠️
editoast/src/views/mod.rs 36.66% 19 Missing ⚠️
editoast/src/tables.rs 76.92% 12 Missing ⚠️
editoast/editoast_authz/src/lib.rs 68.00% 8 Missing ⚠️
editoast/editoast_authz/src/builtin_role.rs 0.00% 6 Missing ⚠️
editoast/src/modelsv2/auth.rs 94.44% 4 Missing ⚠️
editoast/src/views/projects.rs 85.71% 2 Missing ⚠️

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

Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #7981      +/-   ##
============================================
+ Coverage     27.19%   27.41%   +0.21%     
- Complexity     2123     2155      +32     
============================================
  Files          1309     1314       +5     
  Lines        157687   157952     +265     
  Branches       3243     3262      +19     
============================================
+ Hits          42877    43295     +418     
+ Misses       112843   112674     -169     
- Partials       1967     1983      +16     
Flag Coverage Δ
core 75.53% <ø> (+0.09%) ⬆️
editoast 64.48% <79.20%> (+0.22%) ⬆️
front 10.41% <ø> (+0.03%) ⬆️
gateway 2.03% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 72.98% <ø> (-0.20%) ⬇️

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.

@leovalais leovalais force-pushed the users_and_roles branch 3 times, most recently from dcd0bc6 to 3a1f97f Compare July 17, 2024 08:38
@leovalais leovalais force-pushed the users_and_roles branch 7 times, most recently from b59182b to bdffaf1 Compare July 24, 2024 18:44
@leovalais
Copy link
Contributor Author

leovalais commented Jul 24, 2024

On-site peer review with @woshilapin

  • Prefer toml_edit over toml
  • The generic parameter of RoleConfig may be unnecessary
  • The config/config_version in table roles is unnecessary and may cause maintenance issues
  • All application roles should be serialized in DB and garbage collected upon configuration changes. We can ensure the sync either at server boot or manually with a CLI command.

A small design discussion about the last two points will be necessary @Khoyo

@leovalais leovalais changed the title wip: users and roles Role-based authorization system Jul 24, 2024
@leovalais leovalais force-pushed the users_and_roles branch 3 times, most recently from 941e97f to ea79fb2 Compare July 25, 2024 17:28
@leovalais
Copy link
Contributor Author

Changes:

  • builtin roles are now stored in DB with deduplication
  • no more config/config_version
  • simplified requests
  • diesel query builder use
  • lift some StorageDriver constraints
  • StorageDriver functions now take a subject_id instead of an identifier as these operations may be applied to groups
  • StorageDriver is now responsible for serializing and deserializing builtin roles

@woshilapin woshilapin self-assigned this Jul 30, 2024
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.

A first series of comments about editoast_authz. The rest is coming soon, stay tuned.

@leovalais leovalais marked this pull request as ready for review July 31, 2024 11:29
@leovalais leovalais requested review from a team as code owners July 31, 2024 11:29
@leovalais leovalais requested a review from woshilapin July 31, 2024 11:30
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.

Huge PR, looks fine from my side even if I am no expert in authorization & co.

Copy link
Contributor

@kmer2016 kmer2016 left a comment

Choose a reason for hiding this comment

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

LGTM

@leovalais leovalais added this pull request to the merge queue Aug 1, 2024
Merged via the queue into dev with commit bb79451 Aug 1, 2024
20 checks passed
@leovalais leovalais deleted the users_and_roles branch August 1, 2024 10:05
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