Skip to content

Handle API key type in Get/Query API key responses #96014

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
elasticsearchmachine merged 8 commits into from
May 15, 2023
Merged

Handle API key type in Get/Query API key responses #96014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2664531
Handle API key type in Get/Query API key responses
ywangd May 11, 2023
3011716
tweak
ywangd May 11, 2023
0a8ff8a
Merge remote-tracking branch 'origin/main' into rcs-get-special-api-keys
ywangd May 11, 2023
f381182
fix tests
ywangd May 11, 2023
0b13ec4
tweak assertions
ywangd May 11, 2023
65f7dc5
Merge remote-tracking branch 'origin/main' into rcs-get-special-api-keys
ywangd May 12, 2023
f3c58f7
Adding new TransportVersion for serialization change
ywangd May 12, 2023
a7dcea5
Merge branch 'main' into rcs-get-special-api-keys
elasticmachine May 14, 2023
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
3 changes: 2 additions & 1 deletion server/src/main/java/org/elasticsearch/TransportVersion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,12 +121,13 @@ private static TransportVersion registerTransportVersion(int id, String uniqueId
* Detached transport versions added below here.
*/
public static final TransportVersion V_8_500_000 = registerTransportVersion(8_500_000, "dc3cbf06-3ed5-4e1b-9978-ee1d04d235bc");
public static final TransportVersion V_8_500_001 = registerTransportVersion(8_500_001, "c943cfe5-c89d-4eae-989f-f5f4537e84e0");

/**
* Reference to the most recent transport version.
* This should be the transport version with the highest id.
*/
public static final TransportVersion CURRENT = V_8_500_000;
public static final TransportVersion CURRENT = V_8_500_001;

/**
* Reference to the earliest compatible transport version to this version of the codebase.
Expand Down
50 changes: 41 additions & 9 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,6 +13,7 @@
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.common.xcontent.XContentParserUtils;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.transport.TcpTransport;
import org.elasticsearch.xcontent.ConstructingObjectParser;
import org.elasticsearch.xcontent.ObjectParser;
import org.elasticsearch.xcontent.ParseField;
Expand Down Expand Up @@ -80,6 +81,7 @@ public String value() {

private final String name;
private final String id;
private final Type type;
private final Instant creation;
private final Instant expiration;
private final boolean invalidated;
Expand All @@ -94,6 +96,7 @@ public String value() {
public ApiKey(
String name,
String id,
Type type,
Instant creation,
Instant expiration,
boolean invalidated,
Expand All @@ -106,6 +109,7 @@ public ApiKey(
this(
name,
id,
type,
creation,
expiration,
invalidated,
Expand All @@ -120,6 +124,7 @@ public ApiKey(
private ApiKey(
String name,
String id,
Type type,
Instant creation,
Instant expiration,
boolean invalidated,
Expand All @@ -131,6 +136,7 @@ private ApiKey(
) {
this.name = name;
this.id = id;
this.type = type;
// As we do not yet support the nanosecond precision when we serialize to JSON,
// here creating the 'Instant' of milliseconds precision.
// This Instant can then be used for date comparison.
Expand All @@ -153,6 +159,14 @@ public ApiKey(StreamInput in) throws IOException {
this.name = in.readString();
}
this.id = in.readString();
if (in.getTransportVersion().onOrAfter(TransportVersion.V_8_500_001)) {
this.type = in.readEnum(Type.class);
} else {
// This default is safe because
// 1. ApiKey objects never transfer between nodes
// 2. Creating cross-cluster API keys mandates minimal node version that understands the API key type
this.type = Type.REST;
}
this.creation = in.readInstant();
this.expiration = in.readOptionalInstant();
this.invalidated = in.readBoolean();
Expand Down Expand Up @@ -181,6 +195,10 @@ public String getName() {
return name;
}

public Type getType() {
return type;
}

public Instant getCreation() {
return creation;
}
Expand Down Expand Up @@ -221,7 +239,11 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
}

public XContentBuilder innerToXContent(XContentBuilder builder, Params params) throws IOException {
builder.field("id", id).field("name", name).field("creation", creation.toEpochMilli());
builder.field("id", id).field("name", name);
if (TcpTransport.isUntrustedRemoteClusterEnabled()) {
builder.field("type", type.value());
}
Comment on lines +243 to +245
Copy link
Member Author

@ywangd ywangd May 11, 2023

Choose a reason for hiding this comment

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

This PR has some unfortunate complexity in tests due to this conditional serialization logic with featureFlag checking. A simple solution could be adding assumeTrue(featureFlag) for all those tests. But it felt disabling a bit too much. So I took a more finer controlled approach.

Luckily Doc tests do not actually assert Get/Query responses and Yaml tests only assert selective fields. So they need no change in this PR.

builder.field("creation", creation.toEpochMilli());
if (expiration != null) {
builder.field("expiration", expiration.toEpochMilli());
}
Expand All @@ -237,6 +259,7 @@ public XContentBuilder innerToXContent(XContentBuilder builder, Params params) t
builder.endObject();
}
if (limitedBy != null) {
assert type != Type.CROSS_CLUSTER;
builder.field("limited_by", limitedBy);
}
return builder;
Expand All @@ -250,6 +273,9 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeString(name);
}
out.writeString(id);
if (out.getTransportVersion().onOrAfter(TransportVersion.V_8_500_001)) {
out.writeEnum(type);
}
out.writeInstant(creation);
out.writeOptionalInstant(expiration);
out.writeBoolean(invalidated);
Expand All @@ -266,7 +292,7 @@ public void writeTo(StreamOutput out) throws IOException {

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

@Override
Expand All @@ -283,6 +309,7 @@ public boolean equals(Object obj) {
ApiKey other = (ApiKey) obj;
return Objects.equals(name, other.name)
&& Objects.equals(id, other.id)
&& Objects.equals(type, other.type)
&& Objects.equals(creation, other.creation)
&& Objects.equals(expiration, other.expiration)
&& Objects.equals(invalidated, other.invalidated)
Expand All @@ -298,19 +325,22 @@ public boolean equals(Object obj) {
return new ApiKey(
(String) args[0],
(String) args[1],
Instant.ofEpochMilli((Long) args[2]),
(args[3] == null) ? null : Instant.ofEpochMilli((Long) args[3]),
(Boolean) args[4],
(String) args[5],
// TODO: remove null check once TcpTransport.isUntrustedRemoteClusterEnabled() is removed
args[2] == null ? Type.REST : (Type) args[2],
Instant.ofEpochMilli((Long) args[3]),
(args[4] == null) ? null : Instant.ofEpochMilli((Long) args[4]),
(Boolean) args[5],
(String) args[6],
(args[7] == null) ? null : (Map<String, Object>) args[7],
(List<RoleDescriptor>) args[8],
(RoleDescriptorsIntersection) args[9]
(String) args[7],
(args[8] == null) ? null : (Map<String, Object>) args[8],
(List<RoleDescriptor>) args[9],
(RoleDescriptorsIntersection) args[10]
);
});
static {
PARSER.declareString(constructorArg(), new ParseField("name"));
PARSER.declareString(constructorArg(), new ParseField("id"));
PARSER.declareField(optionalConstructorArg(), Type::fromXContent, new ParseField("type"), ObjectParser.ValueType.STRING);
PARSER.declareLong(constructorArg(), new ParseField("creation"));
PARSER.declareLong(optionalConstructorArg(), new ParseField("expiration"));
PARSER.declareBoolean(constructorArg(), new ParseField("invalidated"));
Expand Down Expand Up @@ -339,6 +369,8 @@ public String toString() {
+ name
+ ", id="
+ id
+ ", type="
+ type.value()
+ ", creation="
+ creation
+ ", expiration="
Expand Down
28 changes: 20 additions & 8 deletions ...n/core/src/test/java/org/elasticsearch/xpack/core/security/action/apikey/ApiKeyTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.XContentTestUtils;
import org.elasticsearch.transport.TcpTransport;
import org.elasticsearch.xcontent.ToXContent;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.XContentFactory;
Expand All @@ -32,13 +33,15 @@
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;

public class ApiKeyTests extends ESTestCase {

@SuppressWarnings("unchecked")
public void testXContent() throws IOException {
final String name = randomAlphaOfLengthBetween(4, 10);
final String id = randomAlphaOfLength(20);
final ApiKey.Type type = TcpTransport.isUntrustedRemoteClusterEnabled() ? randomFrom(ApiKey.Type.values()) : ApiKey.Type.REST;
// between 1970 and 2065
final Instant creation = Instant.ofEpochSecond(randomLongBetween(0, 3000000000L), randomLongBetween(0, 999999999));
final Instant expiration = randomBoolean()
Expand All @@ -49,11 +52,14 @@ public void testXContent() throws IOException {
final String realmName = randomAlphaOfLengthBetween(3, 8);
final Map<String, Object> metadata = randomMetadata();
final List<RoleDescriptor> roleDescriptors = randomBoolean() ? null : randomUniquelyNamedRoleDescriptors(0, 3);
final List<RoleDescriptor> limitedByRoleDescriptors = randomUniquelyNamedRoleDescriptors(0, 3);
final List<RoleDescriptor> limitedByRoleDescriptors = type == ApiKey.Type.CROSS_CLUSTER
? null
: randomUniquelyNamedRoleDescriptors(0, 3);

final ApiKey apiKey = new ApiKey(
name,
id,
type,
creation,
expiration,
invalidated,
Expand All @@ -77,6 +83,9 @@ public void testXContent() throws IOException {

assertThat(map.get("name"), equalTo(name));
assertThat(map.get("id"), equalTo(id));
if (TcpTransport.isUntrustedRemoteClusterEnabled()) {
assertThat(map.get("type"), equalTo(type.value()));
}
assertThat(Long.valueOf(map.get("creation").toString()), equalTo(creation.toEpochMilli()));
if (expiration != null) {
assertThat(Long.valueOf(map.get("expiration").toString()), equalTo(expiration.toEpochMilli()));
Expand All @@ -100,12 +109,16 @@ public void testXContent() throws IOException {
}

final var limitedByList = (List<Map<String, Object>>) map.get("limited_by");
assertThat(limitedByList.size(), equalTo(1));
final Map<String, Object> limitedByMap = limitedByList.get(0);
assertThat(limitedByMap.size(), equalTo(limitedByRoleDescriptors.size()));
for (RoleDescriptor roleDescriptor : limitedByRoleDescriptors) {
assertThat(limitedByMap, hasKey(roleDescriptor.getName()));
assertThat(XContentTestUtils.convertToMap(roleDescriptor), equalTo(limitedByMap.get(roleDescriptor.getName())));
if (type != ApiKey.Type.CROSS_CLUSTER) {
assertThat(limitedByList.size(), equalTo(1));
final Map<String, Object> limitedByMap = limitedByList.get(0);
assertThat(limitedByMap.size(), equalTo(limitedByRoleDescriptors.size()));
for (RoleDescriptor roleDescriptor : limitedByRoleDescriptors) {
assertThat(limitedByMap, hasKey(roleDescriptor.getName()));
assertThat(XContentTestUtils.convertToMap(roleDescriptor), equalTo(limitedByMap.get(roleDescriptor.getName())));
}
} else {
assertThat(limitedByList, nullValue());
}
}

Expand All @@ -130,7 +143,6 @@ private ApiKey.Type parseTypeString(String typeString) throws IOException {
}
}

@SuppressWarnings("unchecked")
public static Map<String, Object> randomMetadata() {
return randomFrom(
Map.of(
Expand Down
Loading