Skip to content

Add simplified FGA model and tests for same #1790

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
Merged
JAORMX merged 13 commits into from
Dec 19, 2023
Merged

Add simplified FGA model and tests for same #1790

JAORMX merged 13 commits into from
Dec 19, 2023

Conversation

@evankanderson
Copy link
Member

@evankanderson evankanderson commented Dec 1, 2023 •
edited by JAORMX
Loading

Fixes https://github.com/stacklok/epics/issues/131

Followup to auth model discussion on Nov 30 and #1781

Closes: #1843

@evankanderson
Copy link
Member Author

evankanderson commented Dec 1, 2023

Passing tests look like:

(PASSING) check-inheritance: Checks (50/50 passing) | ListObjects (0/0 passing)

Failing tests look like:

(FAILING) check-inheritance: Checks (34/38 passing) | ListObjects (0/0 passing)
✓ Check(user=user:admin1,relation=creator,object=project:001, context=<nil>)
✓ Check(user=user:admin1,relation=viewer,object=project:001, context=<nil>)
✓ Check(user=user:admin1,relation=repo_updater,object=project:001, context=<nil>)
✓ Check(user=user:admin1,relation=provider_creator,object=project:001, context=<nil>)
✓ Check(user=user:admin1,relation=artifact_updater,object=project:001, context=<nil>)
✓ Check(user=user:admin1,relation=creator,object=project:002, context=<nil>)
✓ Check(user=user:admin1,relation=viewer,object=project:002, context=<nil>)
✓ Check(user=user:admin1,relation=repo_updater,object=project:002, context=<nil>)
✓ Check(user=user:admin1,relation=provider_creator,object=project:002, context=<nil>)
✓ Check(user=user:admin1,relation=artifact_updater,object=project:002, context=<nil>)
✓ Check(user=user:admin2,relation=repo_updater,object=project:001, context=<nil>)
✓ Check(user=user:admin2,relation=provider_creator,object=project:001, context=<nil>)
✓ Check(user=user:admin2,relation=artifact_updater,object=project:001, context=<nil>)
✓ Check(user=user:admin2,relation=creator,object=project:001, context=<nil>)
✓ Check(user=user:admin2,relation=viewer,object=project:001, context=<nil>)
✓ Check(user=user:admin2,relation=creator,object=project:003, context=<nil>)
✓ Check(user=user:admin2,relation=viewer,object=project:003, context=<nil>)
✓ Check(user=user:admin2,relation=repo_updater,object=project:003, context=<nil>)
✓ Check(user=user:admin2,relation=provider_creator,object=project:003, context=<nil>)
✓ Check(user=user:admin2,relation=artifact_updater,object=project:003, context=<nil>)
✓ Check(user=user:nonadmin1,relation=repo_updater,object=project:001, context=<nil>)
✓ Check(user=user:nonadmin1,relation=provider_creator,object=project:001, context=<nil>)
✓ Check(user=user:nonadmin1,relation=artifact_updater,object=project:001, context=<nil>)
✓ Check(user=user:nonadmin1,relation=provider_getter,object=project:001, context=<nil>)
✓ Check(user=user:nonadmin1,relation=creator,object=project:001, context=<nil>)
✓ Check(user=user:nonadmin1,relation=viewer,object=project:001, context=<nil>)
✓ Check(user=user:nonadmin1,relation=artifact_updater,object=project:002, context=<nil>)
✓ Check(user=user:nonadmin1,relation=provider_getter,object=project:002, context=<nil>)
✓ Check(user=user:nonadmin1,relation=creator,object=project:002, context=<nil>)
✓ Check(user=user:nonadmin1,relation=viewer,object=project:002, context=<nil>)
✓ Check(user=user:nonadmin1,relation=repo_updater,object=project:002, context=<nil>)
✓ Check(user=user:nonadmin1,relation=provider_creator,object=project:002, context=<nil>)
ⅹ Check(user=user:nonadmin1,relation=creator,object=project:001, context=<nil>): expected=true, got=false, error=<nil>
✓ Check(user=user:nonadmin1,relation=viewer,object=project:001, context=<nil>)
ⅹ Check(user=user:nonadmin1,relation=repo_updater,object=project:001, context=<nil>): expected=true, got=false, error=<nil>
ⅹ Check(user=user:nonadmin1,relation=provider_creator,object=project:001, context=<nil>): expected=true, got=false, error=<nil>
ⅹ Check(user=user:nonadmin1,relation=artifact_updater,object=project:001, context=<nil>): expected=true, got=false, error=<nil>
✓ Check(user=user:nonadmin1,relation=provider_getter,object=project:001, context=<nil>)

JAORMX
JAORMX reviewed Dec 1, 2023
View reviewed changes
JAORMX
JAORMX reviewed Dec 1, 2023
View reviewed changes
jhrozek
jhrozek reviewed Dec 1, 2023
View reviewed changes
@evankanderson
Copy link
Member Author

evankanderson commented Dec 4, 2023

FYI, I removed the README.md because I figured out that the FGA DSL does support some comments in some places, and was able to move the comment into the model.

I also filed openfga/language#115, because the comment support was clearly half-baked. 😁

JAORMX
JAORMX requested changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

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

Let's rename this directory to authz to denote it doesn't have to do with authentication.

Can you add the license headers to the new files?

Otherwise I think this is good to go! Thanks for researching this!

stacklokbot
stacklokbot reviewed Dec 16, 2023
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

Minder analyzed this PR and found no vulnerable dependencies.

stacklokbot
stacklokbot reviewed Dec 16, 2023
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

Minder analyzed this PR and found no vulnerable dependencies.

stacklokbot
stacklokbot reviewed Dec 16, 2023
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

Minder analyzed this PR and found no vulnerable dependencies.

@evankanderson evankanderson marked this pull request as ready for review December 16, 2023 18:36
stacklokbot
stacklokbot reviewed Dec 16, 2023
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

Minder analyzed this PR and found no vulnerable dependencies.

@evankanderson
Add bootstrap & tests, move to authz/ directory
5e26642 
Address feedback from code review:
- role name -> verb
- add licenses to files
stacklokbot
stacklokbot reviewed Dec 16, 2023
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

Minder analyzed this PR and found no vulnerable dependencies.

stacklokbot
stacklokbot reviewed Dec 16, 2023
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

Minder analyzed this PR and found no vulnerable dependencies.

stacklokbot
stacklokbot reviewed Dec 16, 2023
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

Minder analyzed this PR and found no vulnerable dependencies.

stacklokbot
stacklokbot reviewed Dec 16, 2023
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

Minder analyzed this PR and found no vulnerable dependencies.

JAORMX
JAORMX reviewed Dec 18, 2023
View reviewed changes
Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

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

I really like how the model ended up. Looks quite understandable.

authz/tests/fga_test.go Outdated
t.Run(file, func(t *testing.T) {
t.Parallel()

output, err := exec.Command("fga", "model", "test", "--tests", file).CombinedOutput()
Copy link
Contributor

@JAORMX JAORMX Dec 18, 2023

Choose a reason for hiding this comment

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

We don't do make boostrap before the unit tests. We'd need to start doing that, or run the FGA tests as a separate CI job or test unit.

Copy link
Member Author

@evankanderson evankanderson Dec 18, 2023

Choose a reason for hiding this comment

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

I could also add a go install for the one binary, but it feels like calling make bootstrap might be clearer?

Copy link
Contributor

@JAORMX JAORMX Dec 18, 2023

Choose a reason for hiding this comment

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

It is cleaner, but make bootstrap is also very time consuming. What about separating the FGA tests and have a dedicated target that does the docker invocation?

Copy link
Member Author

@evankanderson evankanderson Dec 18, 2023

Choose a reason for hiding this comment

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

There's no docker invocation here, just the CLI process.

Oh, but I could actually just import the command code... let me try that.

Copy link
Member Author

@evankanderson evankanderson Dec 18, 2023

Choose a reason for hiding this comment

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

Okay, I've fixed this by actually importing the FGA CLI bits and invoking them directly, so we don't even need bootstrap anymore.

The fga_test.go still doesn't realize that it depends on minder.fga, so it will use cached results if you make changes, but that seems like a minor thing that we can figure out later.

@jhrozek jhrozek mentioned this pull request Dec 18, 2023
Closed
@evankanderson evankanderson mentioned this pull request Dec 18, 2023
Merged
@evankanderson evankanderson requested a review from a team December 18, 2023 19:30
@evankanderson evankanderson force-pushed the explore-fga branch 4 times, most recently from ec5932d to 01c1aab Compare December 18, 2023 19:58
evankanderson
evankanderson commented Dec 18, 2023
View reviewed changes
authz/tests/fga_test.go Outdated
// We invoke cobra commands directly, which references some global state in FGA.
// Call .SetEnv here to hint to paralleltest that this can't be parallelized.
// See https://github.com/kunwardeep/paralleltest/pull/33
t.Setenv("PARALLELTEST_NO_PARALLEL", "true")
Copy link
Member Author

@evankanderson evankanderson Dec 18, 2023

Choose a reason for hiding this comment

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

I would love to have a comment or something that can quiet parelleltest, but the code seems to be constructed to parse the AST for methods, rather than comment-driven. kunwardeep/paralleltest#33 provides a hook to do this, though it also forces us to make this test not run at all in parallel with our other tests. It runs in 0.2s on my MacBook, so it's not too bad at the moment.

Copy link
Member Author

@evankanderson evankanderson Dec 18, 2023

Choose a reason for hiding this comment

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

... and golangci-lint hasn't been released to pick up that change yet, so I needed to edit the workflow, too.

Copy link
Member Author

@evankanderson evankanderson Dec 18, 2023

Choose a reason for hiding this comment

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

I just figured out that I can just use a file-level mutex around calls to .Execute(), which is much simpler.

JAORMX
JAORMX previously approved these changes Dec 19, 2023
View reviewed changes
Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

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

I like your new solution a lot better. thanks for working on this!

JAORMX
JAORMX previously approved these changes Dec 19, 2023
View reviewed changes
@JAORMX JAORMX merged commit 1d80f3e into mindersec:main Dec 19, 2023
@evankanderson evankanderson deleted the explore-fga branch July 2, 2025 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jhrozek jhrozek jhrozek left review comments

@JAORMX JAORMX JAORMX approved these changes

+2 more reviewers

@aaguiarz aaguiarz aaguiarz left review comments

@stacklokbot stacklokbot stacklokbot left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add initial OpenFGA model

5 participants

@evankanderson @JAORMX @jhrozek @aaguiarz @stacklokbot