-
Notifications
You must be signed in to change notification settings - Fork 589
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
Define and use roles for authorization #8874
Conversation
f2e5ec7
to
75a3f48
Compare
controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/PathGroup.java
Outdated
Show resolved
Hide resolved
controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/PathGroup.java
Show resolved
Hide resolved
return get(path).filter(p -> { | ||
boolean match = true; | ||
String tenant = p.get("tenant"); | ||
if (tenant != null && context.tenant().isPresent()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it a bit strange that if the context has a tenant, but the path doesn't, it's a "match". Likewise for application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the other way around.
controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/Role.java
Outdated
Show resolved
Hide resolved
controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/Context.java
Show resolved
Hide resolved
* Paths used for deployments by build service(s). Note that context is ignored in these paths as build service | ||
* roles are not granted in specific contexts. | ||
*/ | ||
buildService("/zone/v1/{*}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API is used by users to deploy to dev
.
A better solution would probably be to expose a deploy API path with environment (dev
or perf
) directly under an application, which picks the correct region, but that is some work and not really part of this.
} | ||
|
||
static { | ||
// Ensure that all path spec sets are disjoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this just be a test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I just prefer to keep contracts within the class (similar requireSomething methods) when possible. If validation is a lot of code and/or needs other classes to do the validation, it can be moved to a test.
Set<String> overlapping = new LinkedHashSet<>(pg.pathSpecs); | ||
overlapping.retainAll(pg2.pathSpecs); | ||
if (!overlapping.isEmpty()) { | ||
throw new AssertionError("The following path specs overlap in " + pg + " and " + pg2 + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use junit assertions with message instead of throwing.
&& ! (parts1[i].startsWith("{") && parts1[i].endsWith("}")) | ||
&& ! (parts2[i].startsWith("{") && parts2[i].endsWith("}"))) break; | ||
|
||
if (i == end) throw new AssertionError("Paths '" + path1 + "' and '" + path2 +"' overlap."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
@@ -37,7 +49,7 @@ public String toString() { | |||
* membership to a {@link RoleMembership}. | |||
*/ | |||
public interface Resolver { | |||
RoleMembership membership(); | |||
RoleMembership membership(Principal user); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked it better before, with the resolving inside the resolver. How about making it membership(Optional<Principal>)
? That way the context can be set for the current system, as before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussion with @tokle we agreed the Principal
should not be optional — it is a setup error if this filter is run without a Principal
being set in the request.
LGTM |
Let's see how it fares :) |
There are some minor test updates due to ambiguities in the original
authorization logic.