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.

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

Jump to bottom

Add enabled status for token and api key service #38687

Merged
merged 4 commits into from
Feb 14, 2019
Merged

Add enabled status for token and api key service #38687

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c075aac
Add enabled status for token and api key service
Feb 11, 2019
25082ab
Merge branch 'master' into add-enabled-status-api-token-service
Feb 12, 2019
0c78be6
Use CURRENT version and change it to 7.1.0 once backported
Feb 12, 2019
65fbbb4
Address review comments
Feb 14, 2019
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
18 changes: 17 additions & 1 deletion ...gin/core/src/main/java/org/elasticsearch/xpack/core/security/SecurityFeatureSetUsage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,17 @@ public class SecurityFeatureSetUsage extends XPackFeatureSet.Usage {
private static final String ROLES_XFIELD = "roles";
private static final String ROLE_MAPPING_XFIELD = "role_mapping";
private static final String SSL_XFIELD = "ssl";
private static final String TOKEN_SERVICE_XFIELD = "token_service";
private static final String API_KEY_SERVICE_XFIELD = "api_key_service";
private static final String AUDIT_XFIELD = "audit";
private static final String IP_FILTER_XFIELD = "ipfilter";
private static final String ANONYMOUS_XFIELD = "anonymous";

private Map<String, Object> realmsUsage;
private Map<String, Object> rolesStoreUsage;
private Map<String, Object> sslUsage;
private Map<String, Object> tokenServiceUsage;
private Map<String, Object> apiKeyServiceUsage;
private Map<String, Object> auditUsage;
private Map<String, Object> ipFilterUsage;
private Map<String, Object> anonymousUsage;
Expand All @@ -39,6 +43,10 @@ public SecurityFeatureSetUsage(StreamInput in) throws IOException {
realmsUsage = in.readMap();
rolesStoreUsage = in.readMap();
sslUsage = in.readMap();
if (in.getVersion().onOrAfter(Version.V_7_1_0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I got it right should be Version.CURRENT and do the dance 😢 : commit to master, change to Version.V_7_1_0 in the backport commit, disable bwc tests, commit to 7.x, wait for green intake, then change to Version.V_7_1_0 in the master, and enable bwc tests . If you learn of an easier method lemme know please! I'll admit it, in the past I cut a few corners on this...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you are right, I have changed this to CURRENT with a TODO, will address it on backport to 7.x.
Thank you for your comment.

tokenServiceUsage = in.readMap();
apiKeyServiceUsage = in.readMap();
}
auditUsage = in.readMap();
ipFilterUsage = in.readMap();
if (in.getVersion().before(Version.V_6_0_0_beta1)) {
Expand All @@ -52,12 +60,15 @@ public SecurityFeatureSetUsage(StreamInput in) throws IOException {
public SecurityFeatureSetUsage(boolean available, boolean enabled, Map<String, Object> realmsUsage,
Map<String, Object> rolesStoreUsage, Map<String, Object> roleMappingStoreUsage,
Map<String, Object> sslUsage, Map<String, Object> auditUsage,
Map<String, Object> ipFilterUsage, Map<String, Object> anonymousUsage) {
Map<String, Object> ipFilterUsage, Map<String, Object> anonymousUsage,
Map<String, Object> tokenServiceUsage, Map<String, Object> apiKeyServiceUsage) {
super(XPackField.SECURITY, available, enabled);
this.realmsUsage = realmsUsage;
this.rolesStoreUsage = rolesStoreUsage;
this.roleMappingStoreUsage = roleMappingStoreUsage;
this.sslUsage = sslUsage;
this.tokenServiceUsage = tokenServiceUsage;
this.apiKeyServiceUsage = apiKeyServiceUsage;
this.auditUsage = auditUsage;
this.ipFilterUsage = ipFilterUsage;
this.anonymousUsage = anonymousUsage;
Expand All @@ -69,6 +80,8 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeMap(realmsUsage);
out.writeMap(rolesStoreUsage);
out.writeMap(sslUsage);
out.writeMap(tokenServiceUsage);
Copy link
Member

Choose a reason for hiding this comment

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

@bizybot I missed this in my review but there is a bug here; we write the map always without checking the version. We need the same guards on both reading and writing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True I missed this as well, Thanks for addressing this.

out.writeMap(apiKeyServiceUsage);
out.writeMap(auditUsage);
out.writeMap(ipFilterUsage);
if (out.getVersion().before(Version.V_6_0_0_beta1)) {
Expand All @@ -87,6 +100,8 @@ protected void innerXContent(XContentBuilder builder, Params params) throws IOEx
builder.field(ROLES_XFIELD, rolesStoreUsage);
builder.field(ROLE_MAPPING_XFIELD, roleMappingStoreUsage);
builder.field(SSL_XFIELD, sslUsage);
builder.field(TOKEN_SERVICE_XFIELD, tokenServiceUsage);
builder.field(API_KEY_SERVICE_XFIELD, apiKeyServiceUsage);
builder.field(AUDIT_XFIELD, auditUsage);
builder.field(IP_FILTER_XFIELD, ipFilterUsage);
builder.field(ANONYMOUS_XFIELD, anonymousUsage);
Expand All @@ -96,4 +111,5 @@ protected void innerXContent(XContentBuilder builder, Params params) throws IOEx
public Map<String, Object> getRealmsUsage() {
return Collections.unmodifiableMap(realmsUsage);
}

}
18 changes: 15 additions & 3 deletions ...ck/plugin/security/src/main/java/org/elasticsearch/xpack/security/SecurityFeatureSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@
import java.util.concurrent.atomic.AtomicReference;

import static java.util.Collections.singletonMap;
import static org.elasticsearch.xpack.core.XPackSettings.API_KEY_SERVICE_ENABLED_SETTING;
import static org.elasticsearch.xpack.core.XPackSettings.HTTP_SSL_ENABLED;
import static org.elasticsearch.xpack.core.XPackSettings.TOKEN_SERVICE_ENABLED_SETTING;
import static org.elasticsearch.xpack.core.XPackSettings.TRANSPORT_SSL_ENABLED;

/**
Expand Down Expand Up @@ -93,6 +95,8 @@ public Map<String, Object> nativeCodeInfo() {
@Override
public void usage(ActionListener<XPackFeatureSet.Usage> listener) {
Map<String, Object> sslUsage = sslUsage(settings);
Map<String, Object> tokenServiceUsage = tokenServiceUsage(settings);
Map<String, Object> apiKeyServiceUsage = apiKeyServiceUsage(settings);
Map<String, Object> auditUsage = auditUsage(settings);
Map<String, Object> ipFilterUsage = ipFilterUsage(ipFilter);
Map<String, Object> anonymousUsage = singletonMap("enabled", AnonymousUser.isAnonymousEnabled(settings));
Expand All @@ -103,9 +107,9 @@ public void usage(ActionListener<XPackFeatureSet.Usage> listener) {
final CountDown countDown = new CountDown(3);
final Runnable doCountDown = () -> {
if (countDown.countDown()) {
listener.onResponse(new SecurityFeatureSetUsage(available(), enabled(), realmsUsageRef.get(),
rolesUsageRef.get(), roleMappingUsageRef.get(),
sslUsage, auditUsage, ipFilterUsage, anonymousUsage));
listener.onResponse(new SecurityFeatureSetUsage(available(), enabled(), realmsUsageRef.get(), rolesUsageRef.get(),
roleMappingUsageRef.get(), sslUsage, auditUsage, ipFilterUsage, anonymousUsage, tokenServiceUsage,
apiKeyServiceUsage));
}
};

Expand Down Expand Up @@ -152,6 +156,14 @@ static Map<String, Object> sslUsage(Settings settings) {
return map;
}

static Map<String, Object> tokenServiceUsage(Settings settings) {
return singletonMap("enabled", TOKEN_SERVICE_ENABLED_SETTING.get(settings));
}

static Map<String, Object> apiKeyServiceUsage(Settings settings) {
return singletonMap("enabled", API_KEY_SERVICE_ENABLED_SETTING.get(settings));
}

static Map<String, Object> auditUsage(Settings settings) {
Map<String, Object> map = new HashMap<>(2);
map.put("enabled", XPackSettings.AUDIT_ENABLED.get(settings));
Expand Down
22 changes: 22 additions & 0 deletions ...ugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityFeatureSetTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,20 @@ public void testUsage() throws Exception {
settings.put("xpack.security.http.ssl.enabled", httpSSLEnabled);
final boolean transportSSLEnabled = randomBoolean();
settings.put("xpack.security.transport.ssl.enabled", transportSSLEnabled);

boolean configureEnabledFlagForTokenAndApiKeyServices = randomBoolean();
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be best to separate these two services enabled in the test. That way we wouldn't miss a bug that mistakenly reports the wrong value if one service is enabled and the other is not

final boolean tokenServiceEnabled;
final boolean apiKeyServiceEnabled;
if (configureEnabledFlagForTokenAndApiKeyServices) {
tokenServiceEnabled = randomBoolean();
settings.put("xpack.security.authc.token.enabled", tokenServiceEnabled);
apiKeyServiceEnabled = randomBoolean();
settings.put("xpack.security.authc.api_key.enabled", apiKeyServiceEnabled);
} else {
tokenServiceEnabled = httpSSLEnabled;
apiKeyServiceEnabled = httpSSLEnabled;
}

final boolean auditingEnabled = randomBoolean();
settings.put(XPackSettings.AUDIT_ENABLED.getKey(), auditingEnabled);
final boolean httpIpFilterEnabled = randomBoolean();
Expand Down Expand Up @@ -185,6 +199,12 @@ public void testUsage() throws Exception {
assertThat(source.getValue("ssl.http.enabled"), is(httpSSLEnabled));
assertThat(source.getValue("ssl.transport.enabled"), is(transportSSLEnabled));

// check Token service
assertThat(source.getValue("token_service.enabled"), is(tokenServiceEnabled));

// check API Key service
assertThat(source.getValue("api_key_service.enabled"), is(apiKeyServiceEnabled));

// auditing
assertThat(source.getValue("audit.enabled"), is(auditingEnabled));
if (auditingEnabled) {
Expand Down Expand Up @@ -218,6 +238,8 @@ public void testUsage() throws Exception {
} else {
assertThat(source.getValue("realms"), is(nullValue()));
assertThat(source.getValue("ssl"), is(nullValue()));
assertThat(source.getValue("token_service"), is(nullValue()));
assertThat(source.getValue("api_key_service"), is(nullValue()));
assertThat(source.getValue("audit"), is(nullValue()));
assertThat(source.getValue("anonymous"), is(nullValue()));
assertThat(source.getValue("ipfilter"), is(nullValue()));
Expand Down