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

Add authorizers in virtual workspaces #1186

Merged

Conversation

davidfestal
Copy link
Member

Summary

Add authorizers in virtual workspaces

Related issue(s)

This is a pre-requisite for issues #1172 and #1151

Signed-off-by: David Festal <dfestal@redhat.com>
Signed-off-by: David Festal <dfestal@redhat.com>
Signed-off-by: David Festal <dfestal@redhat.com>
Signed-off-by: David Festal <dfestal@redhat.com>
@davidfestal davidfestal force-pushed the authorizers-in-virtual-workspaces branch from a55eed7 to 66cc1dc Compare June 1, 2022 17:28
Copy link

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

I think the default for the authorizers should be NoOpinion if not set.

This might break some things that rely on this, I wonder if that is worth the price to pay?

if vw.Authorizer != nil {
return vw.Authorizer(ctx, a)
}
return authorizer.DecisionAllow, "", nil

Choose a reason for hiding this comment

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

Should this be NoOpinion?

Copy link
Member Author

Choose a reason for hiding this comment

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

So to avoid breaking anything we'd add an explicit AllowEverything authorizers in all the virtual workspace instances ?

Copy link
Member Author

Choose a reason for hiding this comment

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

At least to make it clear that there's still work to be done ?

Copy link
Member

Choose a reason for hiding this comment

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

we can break non-authorized tests (and ppl working without today). That's totally fine.

Copy link
Member

Choose a reason for hiding this comment

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

Also a vw without authorizer should forbid every request.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed in cf98b01

@davidfestal davidfestal requested a review from sttts June 1, 2022 17:49
... when no autorizer is defined.

Signed-off-by: David Festal <dfestal@redhat.com>
@davidfestal davidfestal merged commit 64c9b48 into kcp-dev:main Jun 2, 2022
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.

3 participants