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

OpenFGA client crate #10504

Merged
merged 3 commits into from
Feb 26, 2025
Merged

OpenFGA client crate #10504

merged 3 commits into from
Feb 26, 2025

Conversation

leovalais
Copy link
Contributor

@leovalais leovalais commented Jan 23, 2025

Note

This is a lengthy PR which also requires some amount of OpenFGA knowledge to be understood. It's probably better to peer review it.

  • Meant to remain a new sub-crate which will be included in editoast_authz (in which we'll map the authorization model, define our objects, their conversion, relations, etc.)
  • The fga binary is necessary to run unit tests for now: https://github.com/openfga/cli
  • So is a running OpenFGA instance

TODO:

  • Implement all the operations we'll need
    • Operations needed for the roles reimplementation
    • POST /stores/{}/list-users
    • POST /stores/{}/stream-list-objects
    • POST /stores/{}/expand
    • POST /stores/{}/read
    • POST /stores/{}/batch-check
    • Other operations currently not needed, but since there's so few of them, we may want to implement them for completeness?
  • Keep an authorization_model_id in the client to forward it to requests—as advised by OpenFGA
  • Improve tests utils & debugging tools
    • Client check assertions (make them public + async variant?)
    • Parse OpenFGA errors instead of just forwarding them generically
    • Expose compile_model?
  • Documentation (for real this time 👀)
    • mod client
    • mod model
    • crate (ongoing)
  • INSTRUMENT EVERYTHING
  • Setup OpenFGA in docker compose
  • Add check for User.id ≠ '*'
  • Add early verification of User.id and Object.id? Not really required for us.
  • Implement list-users as this will change the trait User API
  • Macros
    • fga!
    • relations!
    • derive(User)
    • derive(Object)

About macros

This crates introduces a few macro_rules, which are only used for tests currently.

  • relations! allows defining typed relations concisely to allow quick comparisons with the OpenFGA schema. I think we should keep this one, that'll help the review process and reduce straightforward boilerplate.
  • fga_type! declares the newtype structs representing the types of OpenFGA and generates a few implementations. I think this one should remain a #[cfg(test)] definition. However I think derive macros derive(fga::User, fga::Object) would help us and cost nothing as they are trivial to implement.
  • fga! allows manipulating and instantiating types and relations with the same syntax as OpenFGA, but in a type-safe way. This is particularly useful for the conciseness of unit tests. I think it should be exported.

@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2025

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

Codecov Report

Attention: Patch coverage is 75.44022% with 265 lines in your changes missing coverage. Please review.

Project coverage is 81.68%. Comparing base (bd02b7e) to head (56946bd).
Report is 7 commits behind head on dev.

Files with missing lines Patch % Lines
editoast/fga/src/client.rs 74.13% 224 Missing ⚠️
editoast/fga/src/model.rs 71.15% 30 Missing ⚠️
editoast/fga/src/client/queries.rs 37.50% 5 Missing ⚠️
editoast/fga_derive/src/lib.rs 90.74% 5 Missing ⚠️
editoast/fga/src/client/stores.rs 66.66% 1 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #10504      +/-   ##
==========================================
- Coverage   82.55%   81.68%   -0.88%     
==========================================
  Files        1087     1095       +8     
  Lines      108101   110260    +2159     
  Branches      742      742              
==========================================
+ Hits        89248    90071     +823     
- Misses      18811    20147    +1336     
  Partials       42       42              
Flag Coverage Δ
editoast 72.18% <77.89%> (-2.22%) ⬇️
front 90.38% <ø> (ø)
gateway 2.18% <ø> (ø)
osrdyne 2.98% <0.00%> (-0.31%) ⬇️
railjson_generator 87.58% <ø> (ø)
tests 87.90% <ø> (ø)

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 lva/openfga-client branch 7 times, most recently from 131388a to ac0adfd Compare February 6, 2025 12:27
@leovalais leovalais self-assigned this Feb 6, 2025
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.

Only a first pass on src/client.rs. I'll take a look at the rest later.

@github-actions github-actions bot added the area:editoast Work on Editoast Service label Feb 17, 2025
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.

Small review of the docker-compose

@github-actions github-actions bot removed the area:editoast Work on Editoast Service label Feb 18, 2025
@leovalais leovalais changed the base branch from lva/roles-openfga-edition to dev February 18, 2025 15:23
@sim51
Copy link
Contributor

sim51 commented Feb 19, 2025

I tested the docker integration, and so far it's good (in dev-front and host mode).

As discussed with @flomonster , if you already have a database with some data and you start the stack, you will have some SQL errors from the container openfga-migrate.
To solve it, you need to apply this SQL script :

create schema openfga;
grant all privileges on schema public to osrd;

On a fresh install of this stack (I tested it), the issue is not present: those commands are directly executed by the script docker/init_db.sql which is ran at db creation.

@leovalais leovalais force-pushed the lva/openfga-client branch 2 times, most recently from 2484f16 to 749e0b6 Compare February 20, 2025 15:31
@github-actions github-actions bot added the area:editoast Work on Editoast Service label Feb 20, 2025
@leovalais leovalais marked this pull request as ready for review February 20, 2025 15:44
@leovalais leovalais requested a review from a team as a code owner February 20, 2025 15:44
@leovalais leovalais force-pushed the lva/openfga-client branch 6 times, most recently from 3348b28 to 3b89317 Compare February 21, 2025 15:05
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.

Thanks for all the work on this PR. There are a bunch of unchecked items in the description, do we need to put them in an issue?

@flomonster flomonster force-pushed the lva/openfga-client branch 3 times, most recently from fad9eb6 to c8c61f0 Compare February 21, 2025 16:06
@leovalais
Copy link
Contributor Author

There are a bunch of unchecked items in the description, do we need to put them in an issue?

Probably better, but we'll need those to be done eventually to have the full permission system we envision to work.

leovalais and others added 3 commits February 25, 2025 15:00
Signed-off-by: Leo Valais <[email protected]>
Co-authored-by: Florian Amsallem <[email protected]>
Now openfga uses port 8091.

Signed-off-by: Florian Amsallem <[email protected]>
Copy link
Contributor

@Khoyo Khoyo 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 Feb 25, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 25, 2025
@Khoyo Khoyo added this pull request to the merge queue Feb 26, 2025
Merged via the queue into dev with commit 5011167 Feb 26, 2025
27 checks passed
@Khoyo Khoyo deleted the lva/openfga-client branch February 26, 2025 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:ci Work on Continous Integration Pipeline Service area:editoast Work on Editoast Service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants