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 exception metadata for disabled features #52811

Merged
merged 5 commits into from
Mar 5, 2020
Merged

Add exception metadata for disabled features #52811

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
77bdebe
Add exception metadata for disabled features
tvernum Feb 26, 2020
0c39f54
Revert format changes
tvernum Feb 26, 2020
d3caf0e
Fix imports
tvernum Feb 26, 2020
25538dd
Add comments about string literals
tvernum Mar 5, 2020
2597864
Merge branch 'master' into feature-not-enabled
tvernum Mar 5, 2020
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
4 changes: 3 additions & 1 deletion ...k/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@
import org.elasticsearch.xpack.core.security.authc.support.Hasher;
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
import org.elasticsearch.xpack.core.security.user.User;
import org.elasticsearch.xpack.security.support.FeatureNotEnabledException;
import org.elasticsearch.xpack.security.support.FeatureNotEnabledException.Feature;
import org.elasticsearch.xpack.security.support.SecurityIndexManager;

import javax.crypto.SecretKeyFactory;
Expand Down Expand Up @@ -577,7 +579,7 @@ private void ensureEnabled() {
throw LicenseUtils.newComplianceException("api keys");
}
if (enabled == false) {
throw new IllegalStateException("api keys are not enabled");
throw new FeatureNotEnabledException(Feature.API_KEY_SERVICE, "api keys are not enabled");
}
}

Expand Down
4 changes: 3 additions & 1 deletion ...ck/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@
import org.elasticsearch.xpack.core.security.authc.TokenMetaData;
import org.elasticsearch.xpack.core.security.authc.support.Hasher;
import org.elasticsearch.xpack.core.security.authc.support.TokensInvalidationResult;
import org.elasticsearch.xpack.security.support.FeatureNotEnabledException;
import org.elasticsearch.xpack.security.support.FeatureNotEnabledException.Feature;
import org.elasticsearch.xpack.security.support.SecurityIndexManager;

import javax.crypto.Cipher;
Expand Down Expand Up @@ -1457,7 +1459,7 @@ private void ensureEnabled() {
throw LicenseUtils.newComplianceException("security tokens");
}
if (enabled == false) {
throw new IllegalStateException("security tokens are not enabled");
throw new FeatureNotEnabledException(Feature.TOKEN_SERVICE, "security tokens are not enabled");
}
}

Expand Down
40 changes: 40 additions & 0 deletions ...ty/src/main/java/org/elasticsearch/xpack/security/support/FeatureNotEnabledException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

package org.elasticsearch.xpack.security.support;

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.rest.RestStatus;

public class FeatureNotEnabledException extends ElasticsearchException {

public static final String DISABLED_FEATURE_METADATA = "es.disabled.feature";

/**
* The features names here are constants that form part of our API contract.
* Callers (e.g. Kibana) may be dependent on these strings. Do not change them without consideration of BWC.
*/
public enum Feature {
TOKEN_SERVICE("security_tokens"),
API_KEY_SERVICE("api_keys");

private final String featureName;

Feature(String featureName) {
this.featureName = featureName;
}
}
Comment on lines +20 to +29
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether we should give Feature more attention to make it more formalized, because it seems to be an useful concept for other places as well. By formalization, I mean something like, have it in a separate class, place it in a shared package which is more accessible, name each feature more consistently (more on this below).

Including Feature, we now have three different but related concepts: Feature, Setting, Plugin. Plugin seems to be the highest level, in that it provides one or multiple Features and Settings. I am not sure whether Feature has an 1-to-1 relationship with a Setting. If so, I feel the associated Setting should be reflected in this class and this could be useful for users to act accordingly.

If the relationship between Feature and Setting is more complex, e.g. a feature corresponds to multiple Settings working together, it may need to be more carefully defined and promoted to be a first level concept. I am a bit surprised that Feature is not already an existing concept since both Setting and Plugin sound too technical when talking to users, while Feature seem to be a more accessible term.

I am aware that it is not something inherent to this PR so we don't need to solve it all with this PR. One possible actionable item is to move this class to a more common package for reuse by places other than security.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, no, maybe...

We do have "feature" concepts throughout the codebase. The obvious example being isXyzzyAllowed() on XPackLicenseState. Those are, roughly speaking, features.

I can imagine a world when we had strong model of "Feature" and the license checks where all isFeatureAllowed(Feature feature), and elsewhere isFeatureEnabled(Feature), but we don't and I don't think anyone wants to.

The reason I added an enum here is

  1. So that the feature names were static & defined in a single place, so that clients could rely on the names being a well defined set (at least, per version).
  2. I could have solved that with constant Strings, but then the exception constructor would have been (String, String, Object ...) and there would be ambiguity about which String was the feature name and which was the message.

I think there's something to your suggestion, but progress over perfection - to me, this is a straight forward solution to the problem at hand, and we can evolve it into something else in the future if we need.

Copy link
Member

Choose a reason for hiding this comment

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

Enum is good (at least for now). I agree that we don't wanna stall on something that is not in scope of the problem at hand. My actual question is whether we should move this file to a more common module, e.g. the xpack core module for dependency sake, since I can see it being useful for other xpack modules besides security (similar to how license related files are in core).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My personal view, is that it's a false sense of "reuse".
As it stands:

  1. we don't know if this would be useful elsewhere. We can imagine places, but we have no plans to change them
  2. because of (1), we also don't know which enum values would make sense in the future
  3. we do know that future uses of it would be constrained by not being able to change it significantly because they'd risk breaking clients.

Based on the above, I understand the temptation to put it in core, but there's no clear value, and 1 definite limitation, so it seems like something that feels good for reuse purposes, but actually isn't.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about using it in the "role template validation" if we chose to issue warning instead of error out. Since we have decided it's ok to error out, I currently have no other idea where it could be re-used. So the current form is ok with me.


public FeatureNotEnabledException(Feature feature, String message, Object... args) {
super(message, args);
addMetadata(DISABLED_FEATURE_METADATA, feature.featureName);
}

@Override
public RestStatus status() {
return RestStatus.BAD_REQUEST;
}
}
16 changes: 16 additions & 0 deletions ...gin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

package org.elasticsearch.xpack.security.authc;

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.Version;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.support.PlainActionFuture;
Expand Down Expand Up @@ -38,6 +39,7 @@
import org.elasticsearch.xpack.security.authc.ApiKeyService.ApiKeyRoleDescriptors;
import org.elasticsearch.xpack.security.authc.ApiKeyService.CachedApiKeyHashResult;
import org.elasticsearch.xpack.security.authz.store.NativePrivilegeStore;
import org.elasticsearch.xpack.security.support.FeatureNotEnabledException;
import org.elasticsearch.xpack.security.support.SecurityIndexManager;
import org.elasticsearch.xpack.security.test.SecurityMocks;
import org.junit.After;
Expand All @@ -59,8 +61,10 @@
import java.util.concurrent.Semaphore;
import java.util.concurrent.atomic.AtomicInteger;

import static org.elasticsearch.test.TestMatchers.throwableWithMessage;
import static org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR;
import static org.hamcrest.Matchers.arrayContaining;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
Expand Down Expand Up @@ -435,6 +439,18 @@ public void testGetRolesForApiKey() throws Exception {
}
}

public void testApiKeyServiceDisabled() throws Exception {
final Settings settings = Settings.builder().put(XPackSettings.API_KEY_SERVICE_ENABLED_SETTING.getKey(), false).build();
final ApiKeyService service = createApiKeyService(settings);

ElasticsearchException e = expectThrows(ElasticsearchException.class,
() -> service.getApiKeys(randomAlphaOfLength(6), randomAlphaOfLength(8), null, null, new PlainActionFuture<>()));

assertThat(e, instanceOf(FeatureNotEnabledException.class));
assertThat(e, throwableWithMessage("api keys are not enabled"));
assertThat(e.getMetadata(FeatureNotEnabledException.DISABLED_FEATURE_METADATA), contains("api_keys"));
tvernum marked this conversation as resolved.
Show resolved Hide resolved
}

public void testApiKeyCache() {
final String apiKey = randomAlphaOfLength(16);
Hasher hasher = randomFrom(Hasher.PBKDF2, Hasher.BCRYPT4, Hasher.BCRYPT);
Expand Down
23 changes: 14 additions & 9 deletions ...ugin/security/src/test/java/org/elasticsearch/xpack/security/authc/TokenServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
package org.elasticsearch.xpack.security.authc;

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.ElasticsearchSecurityException;
import org.elasticsearch.Version;
import org.elasticsearch.action.ActionListener;
Expand Down Expand Up @@ -54,6 +55,7 @@
import org.elasticsearch.xpack.core.security.authc.support.TokensInvalidationResult;
import org.elasticsearch.xpack.core.security.user.User;
import org.elasticsearch.xpack.core.watcher.watch.ClockMock;
import org.elasticsearch.xpack.security.support.FeatureNotEnabledException;
import org.elasticsearch.xpack.security.support.SecurityIndexManager;
import org.elasticsearch.xpack.security.test.SecurityMocks;
import org.hamcrest.Matchers;
Expand All @@ -63,7 +65,6 @@
import org.junit.BeforeClass;

import javax.crypto.SecretKey;

import java.io.IOException;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
Expand All @@ -79,8 +80,11 @@
import static java.time.Clock.systemUTC;
import static org.elasticsearch.repositories.blobstore.ESBlobStoreRepositoryIntegTestCase.randomBytes;
import static org.elasticsearch.test.ClusterServiceUtils.setState;
import static org.elasticsearch.test.TestMatchers.throwableWithMessage;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;
import static org.mockito.Matchers.any;
Expand Down Expand Up @@ -560,20 +564,21 @@ public void testTokenServiceDisabled() throws Exception {
.put(XPackSettings.TOKEN_SERVICE_ENABLED_SETTING.getKey(), false)
.build(),
Clock.systemUTC(), client, licenseState, securityContext, securityMainIndex, securityTokensIndex, clusterService);
IllegalStateException e = expectThrows(IllegalStateException.class,
ElasticsearchException e = expectThrows(ElasticsearchException.class,
() -> tokenService.createOAuth2Tokens(null, null, null, true, null));
assertEquals("security tokens are not enabled", e.getMessage());
assertThat(e, throwableWithMessage("security tokens are not enabled"));
assertThat(e, instanceOf(FeatureNotEnabledException.class));
assertThat(e.getMetadata(FeatureNotEnabledException.DISABLED_FEATURE_METADATA), contains("security_tokens"));
tvernum marked this conversation as resolved.
Show resolved Hide resolved

PlainActionFuture<UserToken> future = new PlainActionFuture<>();
tokenService.getAndValidateToken(null, future);
assertNull(future.get());

e = expectThrows(IllegalStateException.class, () -> {
PlainActionFuture<TokensInvalidationResult> invalidateFuture = new PlainActionFuture<>();
tokenService.invalidateAccessToken((String) null, invalidateFuture);
invalidateFuture.actionGet();
});
assertEquals("security tokens are not enabled", e.getMessage());
PlainActionFuture<TokensInvalidationResult> invalidateFuture = new PlainActionFuture<>();
e = expectThrows(ElasticsearchException.class, () -> tokenService.invalidateAccessToken((String) null, invalidateFuture));
assertThat(e, throwableWithMessage("security tokens are not enabled"));
assertThat(e, instanceOf(FeatureNotEnabledException.class));
assertThat(e.getMetadata(FeatureNotEnabledException.DISABLED_FEATURE_METADATA), contains("security_tokens"));
tvernum marked this conversation as resolved.
Show resolved Hide resolved
}

public void testBytesKeyEqualsHashCode() {
Expand Down