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.

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

Add simplified FGA model and tests for same #1790

Merged
merged 13 commits into from
Dec 19, 2023
Merged

Add simplified FGA model and tests for same #1790

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

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
auth/model.fga Outdated Show resolved Hide resolved
JAORMX
JAORMX reviewed Dec 1, 2023
View reviewed changes
auth/model.fga Outdated Show resolved Hide resolved
jhrozek
jhrozek reviewed Dec 1, 2023
View reviewed changes
auth/README.md Outdated Show resolved Hide resolved
@evankanderson
Copy link
Member Author

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

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

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

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

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

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.

tools/go.mod Outdated Show resolved Hide resolved
tools/go.mod Outdated Show resolved Hide resolved
@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 as a code owner 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

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

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

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
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stacklokbot stacklokbot stacklokbot left review comments

@aaguiarz aaguiarz aaguiarz left review comments

@jhrozek jhrozek jhrozek left review comments

@JAORMX JAORMX JAORMX approved these changes

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