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

feat(build/testing): add authz specific integration tests #3163

Merged
merged 14 commits into from
Jun 11, 2024
Merged

Conversation

GeorgeMac
Copy link
Contributor

@GeorgeMac GeorgeMac commented Jun 10, 2024

This PRs does a few things, but primarily it adds ITs for Authz.

In addition, there were a couple features I snuck in to make it all possible:

  1. The ability to add metadata to static tokens though the API
  2. The ability to add metadata to the bootstrap token through config
  3. All JWT claims prefixed with io.flipt.auth are copied into authentication metadata

Update:

The addition of more cases demonstrated a couple bugs, which this PR address:

  1. DeleteNamespace was reporting itself as flag resource type. Meaning you could delete a namespace with flag delete perms.
  2. We were not normalizing empty string for namespace to default namespace when creating requests. Meaning you would have to explicitly say I can do X in namespace "" and "default". I changed this so that it normalizes as expected.

Outstanding:

  • Cases around mutating actions (create, update, delete)
  • Cases specifically for JWT
  • Cases specifically for k8s
  • Add IT to GH workflow matrix

@GeorgeMac GeorgeMac marked this pull request as ready for review June 11, 2024 14:31
@GeorgeMac GeorgeMac requested a review from a team as a code owner June 11, 2024 14:31
internal/cmd/grpc.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

looks great to me! thank you for doing all this! has already proven it was necessary

})

for _, namespace := range integration.Namespaces {
t.Run(fmt.Sprintf("InNamespace(%q)", namespace.Key), func(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
t.Run(fmt.Sprintf("InNamespace(%q)", namespace.Key), func(t *testing.T) {
t.Run(fmt.Sprintf("InNamespace(%q)", namespace.Expected), func(t *testing.T) {

Will prevent "" from showing in output

Copy link
Contributor Author

@GeorgeMac GeorgeMac Jun 11, 2024

Choose a reason for hiding this comment

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

This is to test both default and "" (blank) explicitly.
Otherwise, those two test suites will be duplicately named.

@@ -107,7 +110,7 @@ func (req *UpdateNamespaceRequest) Request() Request {
}

func (req *DeleteNamespaceRequest) Request() Request {
return NewRequest(ResourceFlag, ActionDelete, WithNamespace(req.Key))
return NewRequest(ResourceNamespace, ActionDelete, WithNamespace(req.Key))
Copy link
Collaborator

Choose a reason for hiding this comment

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

💯

Resource: r,
Action: a,
Status: StatusSuccess,
Namespace: DefaultNamespace,
Copy link
Collaborator

Choose a reason for hiding this comment

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

😲

@markphelps markphelps added the automerge Used by Kodiak bot to automerge PRs label Jun 11, 2024
@kodiakhq kodiakhq bot merged commit ca7038d into main Jun 11, 2024
35 checks passed
@kodiakhq kodiakhq bot deleted the gm/authz-its branch June 11, 2024 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Used by Kodiak bot to automerge PRs needs docs Requires documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants