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

User Profile - Detailed errors in hasPrivileges response #89224

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Aug 10, 2022

This PR adds a new errors field in the ProfilehasPrivileges response
to report detailed errors encountered, including missing UIDs. It also
removes the existing errors_uids field since this is redundant after
the change.

ywangd added 2 commits August 10, 2022 14:28
This PR adds a new `errors` field in the ProfilehasPrivileges response
to report detailed errors encountered, including missing UIDs. It also
removes the existing `errors_uids` field since this is redundant after
the change.
@ywangd ywangd added >enhancement :Security/Security Security issues without another label v8.5.0 labels Aug 10, 2022
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Aug 10, 2022
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

Hi @ywangd, I've created a changelog YAML for you.

Comment on lines +52 to +53
// Only compare the keys (profile uids) of the errors, actual error types do not matter
return hasPrivilegeUids.equals(that.hasPrivilegeUids) && errors.keySet().equals(that.errors.keySet());
Copy link
Member Author

Choose a reason for hiding this comment

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

There is no good way to compare Exception objects. Since equals and hashCode are only used in tests (ProfileHasPrivilegesResponseTests), I took a simple approach to just compare the erroneous profile uids.

@ywangd
Copy link
Member Author

ywangd commented Aug 10, 2022

@azasypkin This is the PR for adding detailed errors to the ProfileHasPrivileges API response.

@elastic/clients-team This PR drops an existing errors_uid field and added a new errors field.

@ywangd
Copy link
Member Author

ywangd commented Aug 10, 2022

@elasticmachine run elasticsearch-ci/part-2-fips

Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

LGTM from the Kibana side, thanks!

@ywangd
Copy link
Member Author

ywangd commented Aug 18, 2022

@elasticmachine update branch

@ywangd ywangd added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Aug 18, 2022
@ywangd
Copy link
Member Author

ywangd commented Aug 18, 2022

@elasticmachine run elasticsearch-ci/part-2

@ywangd
Copy link
Member Author

ywangd commented Aug 18, 2022

Failure is unrelated and tracked at #89431

@ywangd
Copy link
Member Author

ywangd commented Aug 18, 2022

@elasticmachine run elasticsearch-ci/part-1-fips

@elasticsearchmachine elasticsearchmachine merged commit 725367e into elastic:main Aug 18, 2022
@ywangd ywangd deleted the errors-field-for-profile-has-privileges branch August 18, 2022 02:07
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Aug 19, 2022
* upstream/main: (265 commits)
  Disable openid connect tests due to missing fixture (elastic#89478)
  Add periodic job for single processor node testing
  Updates to changelog processing after docs redesign (elastic#89463)
  Better support for multi cluster for run task (elastic#89442)
  Mute failing tests (elastic#89465)
  [ML] Performance improvements related to ECS Grok pattern usage (elastic#89424)
  Add source fallback support for date and date_nanos mapped types (elastic#89440)
  Reuse Info in lifecycle step (elastic#89419)
  feature: support metrics for multi value fields (elastic#88818)
  Upgrade OpenTelemetry API and remove workaround (elastic#89438)
  Remove LegacyClusterTaskResultActionListener (elastic#89459)
  Add YAML spec docs about matching errors (elastic#89370)
  Remove redundant cluster upgrade tests for auth tokens (elastic#89417)
  Return 400 error for GetUserPrivileges call with API keys (elastic#89333)
  User Profile - Detailed errors in hasPrivileges response (elastic#89224)
  Rollover min_* conditions docs and highlight (elastic#89434)
  REST tests for percentiles_bucket agg (elastic#88029)
  REST tests for cumulative pipeline aggs (elastic#88966)
  Clean-up file watcher keys. (elastic#89429)
  fix a typo in Security.java (elastic#89248)
  ...

# Conflicts:
#	server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java
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!) >enhancement :Security/Security Security issues without another label Team:Security Meta label for security team v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants