Skip to content

[TEST] Remove token methods from HLRC SecurityClient #85515

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

Merged
merged 8 commits into from
Apr 12, 2022

Conversation

tvernum
Copy link
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
Collaborator

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

@elasticmachine
Copy link
Collaborator

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

Copy link
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

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
Member

Choose a reason for hiding this comment

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

👍

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
Member

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
Contributor Author

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.

Comment on lines -868 to 663
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
Member

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
Member

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
Contributor Author

tvernum commented Apr 12, 2022

@elasticmachine update branch

@tvernum
Copy link
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
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
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Apr 13, 2022
* 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
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
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
Development

Successfully merging this pull request may close these issues.

3 participants