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.

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

Fix responses for the token APIs #54532

Merged
merged 15 commits into from
Apr 16, 2020
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -765,7 +765,7 @@ public Cancellable createTokenAsync(CreateTokenRequest request, RequestOptions o
*/
public InvalidateTokenResponse invalidateToken(InvalidateTokenRequest request, RequestOptions options) throws IOException {
return restHighLevelClient.performRequestAndParseEntity(request, SecurityRequestConverters::invalidateToken, options,
InvalidateTokenResponse::fromXContent, emptySet());
InvalidateTokenResponse::fromXContent, singleton(404));
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 is worth to make the behaviour of deleteXxx and invalidateXxx more consistent. It seems most of them are ignoring 404, e.g. deleteUser. Not suggesting it for this PR, but we could potentially push for more consistency in future work. Currently deleteRoleMapping and invalidateApiKey are not yet with this pattern.

}

/**
Expand All @@ -780,7 +780,7 @@ public InvalidateTokenResponse invalidateToken(InvalidateTokenRequest request, R
public Cancellable invalidateTokenAsync(InvalidateTokenRequest request, RequestOptions options,
ActionListener<InvalidateTokenResponse> listener) {
return restHighLevelClient.performRequestAsyncAndParseEntity(request, SecurityRequestConverters::invalidateToken, options,
InvalidateTokenResponse::fromXContent, listener, emptySet());
InvalidateTokenResponse::fromXContent, listener, singleton(404));
}

/**
Expand Down Expand Up @@ -1013,7 +1013,7 @@ public Cancellable invalidateApiKeyAsync(final InvalidateApiKeyRequest request,
* authenticated TLS session, and it is validated by the PKI realms with {@code delegation.enabled} toggled to {@code true}.<br>
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-delegate-pki-authentication.html"> the
* docs</a> for more details.
*
*
* @param request the request containing the certificate chain
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @return the response from the delegate-pki-authentication API key call
Expand All @@ -1031,7 +1031,7 @@ public DelegatePkiAuthenticationResponse delegatePkiAuthentication(DelegatePkiAu
* {@code true}.<br>
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-delegate-pki-authentication.html"> the
* docs</a> for more details.
*
*
* @param request the request containing the certificate chain
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @param listener the listener to be notified upon request completion
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,10 @@ public final class InvalidateTokenResponse {
PARSER.declareInt(constructorArg(), PREVIOUSLY_INVALIDATED_TOKENS);
PARSER.declareInt(constructorArg(), ERROR_COUNT);
PARSER.declareObjectArray(optionalConstructorArg(), (p, c) -> ElasticsearchException.fromXContent(p), ERRORS);

}

public InvalidateTokenResponse(int invalidatedTokens, int previouslyInvalidatedTokens,
@Nullable List<ElasticsearchException> errors) {
public InvalidateTokenResponse(int invalidatedTokens, int previouslyInvalidatedTokens, @Nullable List<ElasticsearchException> errors) {
this.invalidatedTokens = invalidatedTokens;
this.previouslyInvalidatedTokens = previouslyInvalidatedTokens;
if (null == errors) {
Expand Down
6 changes: 3 additions & 3 deletions x-pack/docs/en/rest-api/security/oidc-logout-api.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
=== OpenID Connect logout API

Submits a request to invalidate a refresh token and an access token that was
generated as a response to a call to `/_security/oidc/authenticate`.
generated as a response to a call to `/_security/oidc/authenticate`.

[[security-api-oidc-logout-request]]
==== {api-request-title}
Expand Down Expand Up @@ -48,7 +48,7 @@ POST /_security/oidc/logout
"refresh_token": "vLBPvmAB6KvwvJZr27cS"
}
--------------------------------------------------
// TEST[catch:unauthorized]
// TEST[catch:request]
Copy link
Member Author

Choose a reason for hiding this comment

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

The (expected for this test) 500 was masked by the 401 that was thrown in

invalidateRefreshToken(request.getRefreshToken(), ActionListener.wrap(ignore -> {

Copy link
Member

Choose a reason for hiding this comment

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

This is necessary because the response changes to 200 and error now happens in the OIDC part as 500?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is necessary because we don't do the login flow so we don't have tokens to invalidate for the logout. We expect that this call would fail for that reason - but it failing with a 401 was a mistake


The following example output of the response contains the URI pointing to the
End Session Endpoint of the OpenID Connect Provider with all the parameters of
Expand All @@ -60,4 +60,4 @@ the Logout Request, as HTTP GET parameters:
"redirect" : "https://op-provider.org/logout?id_token_hint=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c&post_logout_redirect_uri=http%3A%2F%2Foidc-kibana.elastic.co%2Floggedout&state=lGYK0EcSLjqH6pkT5EVZjC6eIW5YCGgywj2sxROO"
}
--------------------------------------------------
// NOTCONSOLE
// NOTCONSOLE
4 changes: 2 additions & 2 deletions x-pack/docs/en/security/authentication/oidc-guide.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -623,7 +623,7 @@ authenticate a user with OpenID Connect:

. Make an HTTP POST request to `_security/oidc/prepare`, authenticating as the `facilitator` user, using the name of the
OpenID Connect realm in the {es} configuration in the request body. For more
details, see
details, see
<<security-api-oidc-prepare-authentication>>.
+
[source,console]
Expand Down Expand Up @@ -675,7 +675,7 @@ POST /_security/oidc/logout
"refresh_token": "vLBPvmAB6KvwvJZr27cS"
}
--------------------------------------------------
// TEST[catch:unauthorized]
// TEST[catch:request]
+
If the realm is configured accordingly, this may result in a response with a `redirect` parameter indicating where
the user needs to be redirected in the OP in order to complete the logout process.
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.common.xcontent.ToXContentObject;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.rest.RestStatus;

import java.io.IOException;
import java.util.Collections;
Expand All @@ -33,9 +34,10 @@ public class TokensInvalidationResult implements ToXContentObject, Writeable {
private final List<String> invalidatedTokens;
private final List<String> previouslyInvalidatedTokens;
private final List<ElasticsearchException> errors;
private RestStatus restStatus;

public TokensInvalidationResult(List<String> invalidatedTokens, List<String> previouslyInvalidatedTokens,
@Nullable List<ElasticsearchException> errors) {
@Nullable List<ElasticsearchException> errors, RestStatus restStatus) {
Objects.requireNonNull(invalidatedTokens, "invalidated_tokens must be provided");
this.invalidatedTokens = invalidatedTokens;
Objects.requireNonNull(previouslyInvalidatedTokens, "previously_invalidated_tokens must be provided");
Expand All @@ -45,6 +47,7 @@ public TokensInvalidationResult(List<String> invalidatedTokens, List<String> pre
} else {
this.errors = Collections.emptyList();
}
this.restStatus = restStatus;
}

public TokensInvalidationResult(StreamInput in) throws IOException {
Expand All @@ -54,10 +57,13 @@ public TokensInvalidationResult(StreamInput in) throws IOException {
if (in.getVersion().before(Version.V_7_2_0)) {
in.readVInt();
}
if (in.getVersion().onOrAfter(Version.V_8_0_0)) {
this.restStatus = RestStatus.readFrom(in);
}
}

public static TokensInvalidationResult emptyResult() {
return new TokensInvalidationResult(Collections.emptyList(), Collections.emptyList(), Collections.emptyList());
public static TokensInvalidationResult emptyResult(RestStatus restStatus) {
return new TokensInvalidationResult(Collections.emptyList(), Collections.emptyList(), Collections.emptyList(), restStatus);
}


Expand All @@ -73,6 +79,10 @@ public List<ElasticsearchException> getErrors() {
return errors;
}

public RestStatus getRestStatus() {
return restStatus;
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject()
Expand Down Expand Up @@ -100,5 +110,8 @@ public void writeTo(StreamOutput out) throws IOException {
if (out.getVersion().before(Version.V_7_2_0)) {
out.writeVInt(5);
}
if (out.getVersion().onOrAfter(Version.V_8_0_0)) {
RestStatus.writeTo(out, restStatus);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.core.security.authc.support.TokensInvalidationResult;

Expand All @@ -29,7 +30,8 @@ public void testSerialization() throws IOException {
TokensInvalidationResult result = new TokensInvalidationResult(Arrays.asList(generateRandomStringArray(20, 15, false)),
Arrays.asList(generateRandomStringArray(20, 15, false)),
Arrays.asList(new ElasticsearchException("foo", new IllegalArgumentException("this is an error message")),
new ElasticsearchException("bar", new IllegalArgumentException("this is an error message2"))));
new ElasticsearchException("bar", new IllegalArgumentException("this is an error message2"))),
RestStatus.OK);
InvalidateTokenResponse response = new InvalidateTokenResponse(result);
try (BytesStreamOutput output = new BytesStreamOutput()) {
response.writeTo(output);
Expand All @@ -45,7 +47,7 @@ public void testSerialization() throws IOException {
}

result = new TokensInvalidationResult(Arrays.asList(generateRandomStringArray(20, 15, false)),
Arrays.asList(generateRandomStringArray(20, 15, false)), Collections.emptyList());
Arrays.asList(generateRandomStringArray(20, 15, false)), Collections.emptyList(), RestStatus.OK);
response = new InvalidateTokenResponse(result);
try (BytesStreamOutput output = new BytesStreamOutput()) {
response.writeTo(output);
Expand All @@ -64,7 +66,7 @@ public void testToXContent() throws IOException {
List previouslyInvalidatedTokens = Arrays.asList(generateRandomStringArray(20, 15, false));
TokensInvalidationResult result = new TokensInvalidationResult(invalidatedTokens, previouslyInvalidatedTokens,
Arrays.asList(new ElasticsearchException("foo", new IllegalArgumentException("this is an error message")),
new ElasticsearchException("bar", new IllegalArgumentException("this is an error message2"))));
new ElasticsearchException("bar", new IllegalArgumentException("this is an error message2"))), RestStatus.OK);
InvalidateTokenResponse response = new InvalidateTokenResponse(result);
XContentBuilder builder = XContentFactory.jsonBuilder();
response.toXContent(builder, ToXContent.EMPTY_PARAMS);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ public void doRun() {
indicesWithTokens.add(securityMainIndex.aliasName());
}
if (indicesWithTokens.isEmpty()) {
markComplete();
Copy link
Member

Choose a reason for hiding this comment

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

This is a good catch.

return;
}
DeleteByQueryRequest expiredDbq = new DeleteByQueryRequest(indicesWithTokens.toArray(new String[0]));
Expand Down
Loading