Skip to content

Limited-by role descriptors in Get/QueryApiKey response #89273

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

Merged
merged 3 commits into from
Aug 12, 2022
Merged

Limited-by role descriptors in Get/QueryApiKey response #89273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7218fe9
Limited-by role descriptors in Get/QueryApiKey response
ywangd Aug 10, 2022
7d1cc03
Update docs/changelog/89273.yaml
ywangd Aug 11, 2022
0cfa262
address feedback
ywangd Aug 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/89273.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 89273
summary: Limited-by role descriptors in Get/QueryApiKey response
area: Security
type: enhancement
issues: []
65 changes: 60 additions & 5 deletions ...plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/apikey/ApiKey.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,20 @@
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.xcontent.ConstructingObjectParser;
import org.elasticsearch.xcontent.ObjectParser;
import org.elasticsearch.xcontent.ParseField;
import org.elasticsearch.xcontent.ToXContentObject;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.XContentParser;
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
import org.elasticsearch.xpack.core.security.authz.RoleDescriptorsIntersection;

import java.io.IOException;
import java.time.Instant;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;

import static org.elasticsearch.xcontent.ConstructingObjectParser.constructorArg;
import static org.elasticsearch.xcontent.ConstructingObjectParser.optionalConstructorArg;
Expand All @@ -43,6 +46,8 @@ public final class ApiKey implements ToXContentObject, Writeable {
private final Map<String, Object> metadata;
@Nullable
private final List<RoleDescriptor> roleDescriptors;
@Nullable
private final RoleDescriptorsIntersection limitedBy;

public ApiKey(
String name,
Expand All @@ -53,7 +58,34 @@ public ApiKey(
String username,
String realm,
@Nullable Map<String, Object> metadata,
@Nullable List<RoleDescriptor> roleDescriptors
@Nullable List<RoleDescriptor> roleDescriptors,
@Nullable List<RoleDescriptor> limitedByRoleDescriptors
) {
this(
name,
id,
creation,
expiration,
invalidated,
username,
realm,
metadata,
roleDescriptors,
limitedByRoleDescriptors == null ? null : new RoleDescriptorsIntersection(List.of(Set.copyOf(limitedByRoleDescriptors)))
);
}

private ApiKey(
String name,
String id,
Instant creation,
Instant expiration,
boolean invalidated,
String username,
String realm,
@Nullable Map<String, Object> metadata,
@Nullable List<RoleDescriptor> roleDescriptors,
@Nullable RoleDescriptorsIntersection limitedBy
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 added this new RoleDescriptorsIntersection mainly for future proof (in case we support derived keys properly). It also mirrors what we did with RoleReferenceIntesection and LimitedRole.
It does also introduce some code level overhead. Let me know if you dislike it and prefer a simpler List<RoleDescriptor>.

Copy link
Contributor

Choose a reason for hiding this comment

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

Missed this comment. RoleDescriptorsIntersection makes sense to me. We know we will need something like this and all the work is already there now 👍

) {
this.name = name;
this.id = id;
Expand All @@ -66,7 +98,10 @@ public ApiKey(
this.username = username;
this.realm = realm;
this.metadata = metadata == null ? Map.of() : metadata;
this.roleDescriptors = roleDescriptors;
this.roleDescriptors = roleDescriptors != null ? List.copyOf(roleDescriptors) : null;
// This assertion will need to be changed (or removed) when derived keys are properly supported
assert limitedBy == null || limitedBy.roleDescriptorsList().size() == 1 : "can only have one set of limited-by role descriptors";
this.limitedBy = limitedBy;
}

public ApiKey(StreamInput in) throws IOException {
Expand All @@ -89,8 +124,10 @@ public ApiKey(StreamInput in) throws IOException {
if (in.getVersion().onOrAfter(Version.V_8_5_0)) {
final List<RoleDescriptor> roleDescriptors = in.readOptionalList(RoleDescriptor::new);
this.roleDescriptors = roleDescriptors != null ? List.copyOf(roleDescriptors) : null;
this.limitedBy = in.readOptionalWriteable(RoleDescriptorsIntersection::new);
} else {
this.roleDescriptors = null;
this.limitedBy = null;
}
}

Expand Down Expand Up @@ -130,6 +167,10 @@ public List<RoleDescriptor> getRoleDescriptors() {
return roleDescriptors;
}

public RoleDescriptorsIntersection getLimitedBy() {
return limitedBy;
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
Expand All @@ -153,6 +194,9 @@ public XContentBuilder innerToXContent(XContentBuilder builder, Params params) t
}
builder.endObject();
}
if (limitedBy != null) {
builder.field("limited_by", limitedBy);
}
return builder;
}

Expand All @@ -174,12 +218,13 @@ public void writeTo(StreamOutput out) throws IOException {
}
if (out.getVersion().onOrAfter(Version.V_8_5_0)) {
out.writeOptionalCollection(roleDescriptors);
out.writeOptionalWriteable(limitedBy);
}
}

@Override
public int hashCode() {
return Objects.hash(name, id, creation, expiration, invalidated, username, realm, metadata, roleDescriptors);
return Objects.hash(name, id, creation, expiration, invalidated, username, realm, metadata, roleDescriptors, limitedBy);
}

@Override
Expand All @@ -202,7 +247,8 @@ public boolean equals(Object obj) {
&& Objects.equals(username, other.username)
&& Objects.equals(realm, other.realm)
&& Objects.equals(metadata, other.metadata)
&& Objects.equals(roleDescriptors, other.roleDescriptors);
&& Objects.equals(roleDescriptors, other.roleDescriptors)
&& Objects.equals(limitedBy, other.limitedBy);
}

@SuppressWarnings("unchecked")
Expand All @@ -216,7 +262,8 @@ public boolean equals(Object obj) {
(String) args[5],
(String) args[6],
(args[7] == null) ? null : (Map<String, Object>) args[7],
(List<RoleDescriptor>) args[8]
(List<RoleDescriptor>) args[8],
(RoleDescriptorsIntersection) args[9]
);
});
static {
Expand All @@ -232,6 +279,12 @@ public boolean equals(Object obj) {
p.nextToken();
return RoleDescriptor.parse(n, p, false);
}, new ParseField("role_descriptors"));
PARSER.declareField(
optionalConstructorArg(),
(p, c) -> RoleDescriptorsIntersection.fromXContent(p),
new ParseField("limited_by"),
ObjectParser.ValueType.OBJECT_ARRAY
);
}

public static ApiKey fromXContent(XContentParser parser) throws IOException {
Expand All @@ -258,6 +311,8 @@ public String toString() {
+ metadata
+ ", role_descriptors="
+ roleDescriptors
+ ", limited_by="
+ limitedBy
+ "]";
}

Expand Down
103 changes: 88 additions & 15 deletions ...e/src/main/java/org/elasticsearch/xpack/core/security/action/apikey/GetApiKeyRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,7 @@ public final class GetApiKeyRequest extends ActionRequest {
private final String apiKeyId;
private final String apiKeyName;
private final boolean ownedByAuthenticatedUser;

public GetApiKeyRequest() {
this(null, null, null, null, false);
}
private final boolean withLimitedBy;

public GetApiKeyRequest(StreamInput in) throws IOException {
super(in);
Expand All @@ -46,20 +43,27 @@ public GetApiKeyRequest(StreamInput in) throws IOException {
} else {
ownedByAuthenticatedUser = false;
}
if (in.getVersion().onOrAfter(Version.V_8_5_0)) {
withLimitedBy = in.readBoolean();
} else {
withLimitedBy = false;
}
}

public GetApiKeyRequest(
private GetApiKeyRequest(
@Nullable String realmName,
@Nullable String userName,
@Nullable String apiKeyId,
@Nullable String apiKeyName,
boolean ownedByAuthenticatedUser
boolean ownedByAuthenticatedUser,
boolean withLimitedBy
) {
this.realmName = textOrNull(realmName);
this.userName = textOrNull(userName);
this.apiKeyId = textOrNull(apiKeyId);
this.apiKeyName = textOrNull(apiKeyName);
this.ownedByAuthenticatedUser = ownedByAuthenticatedUser;
this.withLimitedBy = withLimitedBy;
}

private static String textOrNull(@Nullable String arg) {
Expand All @@ -86,22 +90,28 @@ public boolean ownedByAuthenticatedUser() {
return ownedByAuthenticatedUser;
}

public boolean withLimitedBy() {
return withLimitedBy;
}

/**
* Creates get API key request for given realm name
* @param realmName realm name
* @return {@link GetApiKeyRequest}
*/
@Deprecated
public static GetApiKeyRequest usingRealmName(String realmName) {
return new GetApiKeyRequest(realmName, null, null, null, false);
return new GetApiKeyRequest(realmName, null, null, null, false, false);
}

/**
* Creates get API key request for given user name
* @param userName user name
* @return {@link GetApiKeyRequest}
*/
@Deprecated
public static GetApiKeyRequest usingUserName(String userName) {
return new GetApiKeyRequest(null, userName, null, null, false);
return new GetApiKeyRequest(null, userName, null, null, false, false);
}

/**
Expand All @@ -110,8 +120,9 @@ public static GetApiKeyRequest usingUserName(String userName) {
* @param userName user name
* @return {@link GetApiKeyRequest}
*/
@Deprecated
public static GetApiKeyRequest usingRealmAndUserName(String realmName, String userName) {
return new GetApiKeyRequest(realmName, userName, null, null, false);
return new GetApiKeyRequest(realmName, userName, null, null, false, false);
}

/**
Expand All @@ -121,8 +132,9 @@ public static GetApiKeyRequest usingRealmAndUserName(String realmName, String us
* {@code false}
* @return {@link GetApiKeyRequest}
*/
@Deprecated
public static GetApiKeyRequest usingApiKeyId(String apiKeyId, boolean ownedByAuthenticatedUser) {
return new GetApiKeyRequest(null, null, apiKeyId, null, ownedByAuthenticatedUser);
return new GetApiKeyRequest(null, null, apiKeyId, null, ownedByAuthenticatedUser, false);
}

/**
Expand All @@ -133,21 +145,23 @@ public static GetApiKeyRequest usingApiKeyId(String apiKeyId, boolean ownedByAut
* @return {@link GetApiKeyRequest}
*/
public static GetApiKeyRequest usingApiKeyName(String apiKeyName, boolean ownedByAuthenticatedUser) {
return new GetApiKeyRequest(null, null, null, apiKeyName, ownedByAuthenticatedUser);
return new GetApiKeyRequest(null, null, null, apiKeyName, ownedByAuthenticatedUser, false);
}

/**
* Creates get api key request to retrieve api key information for the api keys owned by the current authenticated user.
*/
@Deprecated
public static GetApiKeyRequest forOwnedApiKeys() {
return new GetApiKeyRequest(null, null, null, null, true);
return new GetApiKeyRequest(null, null, null, null, true, false);
}

/**
* Creates get api key request to retrieve api key information for all api keys if the authenticated user is authorized to do so.
*/
@Deprecated
public static GetApiKeyRequest forAllApiKeys() {
return new GetApiKeyRequest();
return GetApiKeyRequest.builder().build();
}

@Override
Expand Down Expand Up @@ -185,6 +199,9 @@ public void writeTo(StreamOutput out) throws IOException {
if (out.getVersion().onOrAfter(Version.V_7_4_0)) {
out.writeOptionalBoolean(ownedByAuthenticatedUser);
}
if (out.getVersion().onOrAfter(Version.V_8_5_0)) {
out.writeBoolean(withLimitedBy);
}
}

@Override
Expand All @@ -200,11 +217,67 @@ public boolean equals(Object o) {
&& Objects.equals(realmName, that.realmName)
&& Objects.equals(userName, that.userName)
&& Objects.equals(apiKeyId, that.apiKeyId)
&& Objects.equals(apiKeyName, that.apiKeyName);
&& Objects.equals(apiKeyName, that.apiKeyName)
&& withLimitedBy == that.withLimitedBy;
}

@Override
public int hashCode() {
return Objects.hash(realmName, userName, apiKeyId, apiKeyName, ownedByAuthenticatedUser);
return Objects.hash(realmName, userName, apiKeyId, apiKeyName, ownedByAuthenticatedUser, withLimitedBy);
}

public static Builder builder() {
return new Builder();
}

public static class Builder {
private String realmName = null;
private String userName = null;
private String apiKeyId = null;
private String apiKeyName = null;
private boolean ownedByAuthenticatedUser = false;
private boolean withLimitedBy = false;
Comment on lines +233 to +239
Copy link
Member Author

Choose a reason for hiding this comment

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

Added a builder class and deprecated the various convenient methods. I think this request class has come to a point that a Builder class is justified. I will have a follow-up PR to remove those deprecated methods.


public Builder realmName(String realmName) {
this.realmName = realmName;
return this;
}

public Builder userName(String userName) {
this.userName = userName;
return this;
}

public Builder apiKeyId(String apiKeyId) {
this.apiKeyId = apiKeyId;
return this;
}

public Builder apiKeyName(String apiKeyName) {
this.apiKeyName = apiKeyName;
return this;
}

public Builder ownedByAuthenticatedUser() {
return ownedByAuthenticatedUser(true);
}

public Builder ownedByAuthenticatedUser(boolean ownedByAuthenticatedUser) {
this.ownedByAuthenticatedUser = ownedByAuthenticatedUser;
return this;
}

public Builder withLimitedBy() {
return withLimitedBy(true);
}

public Builder withLimitedBy(boolean withLimitedBy) {
this.withLimitedBy = withLimitedBy;
return this;
}

public GetApiKeyRequest build() {
return new GetApiKeyRequest(realmName, userName, apiKeyId, apiKeyName, ownedByAuthenticatedUser, withLimitedBy);
}
}
}
Loading