Skip to content

[TEST] Remove token methods from HLRC SecurityClient#85515

Merged
tvernum merged 8 commits intoelastic:masterfrom
tvernum:hlrc/remove-token-methods
Apr 12, 2022
Merged

[TEST] Remove token methods from HLRC SecurityClient#85515
tvernum merged 8 commits intoelastic:masterfrom
tvernum:hlrc/remove-token-methods

Conversation

@tvernum
Copy link
Copy Markdown
Contributor

@tvernum tvernum commented Mar 31, 2022

The High Level Rest Client (HLRC) is deprecated and is no longer
shipped as a standalone artifact.
It can now be safely removed from the ES codebase.

This commit removes all token methods from the HLRC
SecurityClient, along with their associated Request/Response
classes.

Test code that depended on these methods has been migrated
to use TestSecurityClient instead.

Relates: #83423

@tvernum tvernum added >test Issues or PRs that are addressing/adding tests :Clients/Java High Level REST Client :Security/Security Security issues without another label v8.3.0 labels Mar 31, 2022
@tvernum tvernum requested a review from ywangd March 31, 2022 00:59
@elasticmachine elasticmachine added Team:Security Meta label for security team Team:Clients Meta label for clients team labels Mar 31, 2022
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Mar 31, 2022

Pinging @elastic/es-security (Team:Security)

@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Mar 31, 2022

Pinging @elastic/clients-team (Team:Clients)

ywangd
ywangd approved these changes Mar 31, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

LGTM

x-pack/plugin/security/src/test/java/org/elasticsearch/test/TestSecurityClient.java
Comment on lines +340 to +342
// This API returns 404 (with the same body as a 200 response) if there's nothing to delete.
// RestClient will throw an exception on 404, but we don't want that, we want to parse the body and return it
request.addParameter("ignore", "404");
Copy link
Copy Markdown
Member

@ywangd ywangd Mar 31, 2022

Choose a reason for hiding this comment

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

👍

x-pack/plugin/security/src/test/java/org/elasticsearch/test/TestSecurityClient.java
Comment on lines +387 to +395
public record OAuth2Token(String accessToken, Optional<String> refreshToken, String principal) {

@Nullable
public String getRefreshToken() {
return refreshToken.orElse(null);
}
}

public record TokenInvalidation(int invalidated, int previouslyInvalidated, List<ElasticsearchException> errors) {}
Copy link
Copy Markdown
Member

@ywangd ywangd Mar 31, 2022

Choose a reason for hiding this comment

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

I suspect we eventually want to turn these into production code. But it can wait.

Copy link
Copy Markdown
Contributor Author

@tvernum tvernum Apr 11, 2022

Choose a reason for hiding this comment

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

We have TokensInvalidationResult but it contains a list of token ids, not a count of them.
I'm not sure whether we need a production record for the rest response, but if we do we'll move it where it's needed.

...src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/TokenAuthIntegTests.java
Comment on lines 868 to 663
createTokenRequest1.setGrantType("client_credentials");
final PlainActionFuture<org.elasticsearch.xpack.core.security.action.token.CreateTokenResponse> future1 = new PlainActionFuture<>();
final PlainActionFuture<CreateTokenResponse> future1 = new PlainActionFuture<>();
runAsClient.execute(CreateTokenAction.INSTANCE, createTokenRequest1, future1);
Copy link
Copy Markdown
Member

@ywangd ywangd Mar 31, 2022

Choose a reason for hiding this comment

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

This method should be rewritten to use the new convenient methods from TestSecurityClient. I can follow up with a separate PR (since I wrote the original version).

Copy link
Copy Markdown
Member

@ywangd ywangd Apr 12, 2022

Choose a reason for hiding this comment

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

PR is #85819

@tvernum tvernum added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Apr 12, 2022
@tvernum
Copy link
Copy Markdown
Contributor Author

tvernum commented Apr 12, 2022

@elasticmachine update branch

@tvernum
Copy link
Copy Markdown
Contributor Author

tvernum commented Apr 12, 2022

elasticsearch-ci/part-1-fips is failing due to #85803.
Ignoring as it's unrelated to this change.

@tvernum tvernum merged commit 383aac2 into elastic:master Apr 12, 2022
@tvernum tvernum deleted the hlrc/remove-token-methods branch April 12, 2022 07:59
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Apr 12, 2022
@ywangd
More usage of TestSecurityClient in TokenService
19fa529 
This is a follow up PR for elastic#85515 to rewrite one more test to leverage
the new TestSecurityClient and its convenient token methods.

Relates: elastic#85515
@ywangd ywangd mentioned this pull request Apr 12, 2022
Merged
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Apr 13, 2022
@weizijun
Merge branch 'upstream/master' into fix-time_series-tisdord-exception
5dea773 
* upstream/master: (40 commits)
  Fix BuildTests serialization (elastic#85827)
  Use urgent priority for node shutdown cluster state update (elastic#85838)
  Remove Task classes from HLRC (elastic#85835)
  Remove unused migration classes (elastic#85834)
  Remove uses of Charset name parsing (elastic#85795)
  Remove legacy versioned logic for DefaultSystemMemoryInfo (elastic#85761)
  Expose proxy settings for GCS repositories (elastic#85785)
  Remove SLM classes from HLRC (elastic#85825)
  TSDB: fix the time_series in order collect priority (elastic#85526)
  Remove ILM classes from HLRC (elastic#85822)
  FastVectorHighlighter should use ValueFetchers to load source data (elastic#85815)
  Iteratively execute synchronous ingest processors (elastic#84250)
  Remove TransformClient from HLRC  (elastic#85787)
  Mute XPackRestIT deprecation/10_basic/Test Deprecations (elastic#85807)
  Unmute Lintian packaging test (elastic#85778)
  Add a highlighter unit test base class (elastic#85719)
  Remove NIO Transport Plugin (elastic#82085)
  [TEST] Remove token methods from HLRC SecurityClient (elastic#85515)
  [Test] Use thread-safe hashSet for result collection (elastic#85653)
  [TEST] Mute BuildTests.testSerialization (elastic#85801)
  ...

# Conflicts:
#	server/src/test/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcherTests.java
ywangd added a commit that referenced this pull request Apr 26, 2022
@ywangd
More usage of TestSecurityClient in TokenService (#85819)
a74f4b5 
This is a follow up PR for #85515 to rewrite one more test to leverage
the new TestSecurityClient and its convenient token methods.

Relates: #85515
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ywangd ywangd ywangd approved these changes

Assignees

No one assigned

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Security/Security Security issues without another label Team:Clients Meta label for clients team Team:Security Meta label for security team >test Issues or PRs that are addressing/adding tests v8.3.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tvernum @elasticmachine @ywangd