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

[Security/APIKey Service] Internal API endpoint do determine if user has API keys #172884

Merged

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Dec 7, 2023

Summary

This PR enhances the Security server plugin APIKey service with a new method to determine if the user has API keys. The service is integrated into the "No Data Page" server plugin as a new HTTP route. This route is called when needed to direct users effectively through their getting started experience.

Pulled from #172225

Context

...we [sh]ould introduce whatever functionality we required into the security plugin's API Key Service.

Checklist

Delete any items that are not applicable to this PR.

@tsullivan tsullivan force-pushed the security/api-keys-service-has-api-keys branch 3 times, most recently from ad2335c to 12f4747 Compare December 14, 2023 18:38
@tsullivan tsullivan force-pushed the security/api-keys-service-has-api-keys branch from cb840c9 to d6ebe83 Compare December 14, 2023 19:42
@tsullivan tsullivan force-pushed the security/api-keys-service-has-api-keys branch from d6ebe83 to 89a92fd Compare December 14, 2023 19:44
@tsullivan tsullivan changed the title [Security/APIKey Service] Add functionality to determine if user has created API keys [Security/APIKey Service] Internal API endpoint do determine if user has API keys Dec 14, 2023
@tsullivan tsullivan marked this pull request as ready for review December 14, 2023 19:46
@tsullivan tsullivan requested a review from a team as a code owner December 14, 2023 19:46
@tsullivan tsullivan added the release_note:skip Skip the PR/issue when compiling release notes label Dec 14, 2023

const apiResponse = await esClient.asCurrentUser.security.getApiKey({
owner: true,
});
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure the owner field needs any conditions, but perhaps I'm not aware.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'm not aware of any conditions either.

@azasypkin azasypkin self-requested a review December 18, 2023 11:38
@azasypkin
Copy link
Member

ACK: will review today

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.

Looks good! Just left a few comments.

x-pack/plugins/security/server/routes/api_keys/has.ts Outdated Show resolved Hide resolved
x-pack/plugins/security/server/routes/api_keys/has.ts Outdated Show resolved Hide resolved
x-pack/plugins/security/server/routes/api_keys/has.ts Outdated Show resolved Hide resolved

const apiResponse = await esClient.asCurrentUser.security.getApiKey({
owner: true,
});
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'm not aware of any conditions either.

owner: true,
});

const validKeys = apiResponse.api_keys.filter(({ invalidated }) => !invalidated);
Copy link
Member

Choose a reason for hiding this comment

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

question: you filter out invalidated keys, but not expired ones - is it intentional? If not, can we use active_only request parameter to avoid this additional filtering?

Copy link
Member Author

Choose a reason for hiding this comment

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

@azasypkin I see your point, but I don't see the active_only parameter supported in the SecurityGetApiKeyRequest typings.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, it should be available since 8.10.0 - elastic/elasticsearch#98259 - let me figure this out. In the worst case I'd prefer to do something like this to filter keys on ES side instead:

const apiResponse = await esClient.asCurrentUser.security.getApiKey({
  owner: true,
  // @ts-expect-error @elastic/elasticsearch SecurityGetApiKeyRequest.active_only: boolean | undefined
  active_only: true,
});

return response.ok<HasAPIKeysResult>({
  body: {
    hasApiKeys: apiResponse.api_keys.length > 0,
  },
});

Copy link
Member

Choose a reason for hiding this comment

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

Okay, we'll get this definition in the client soon (elastic/elasticsearch#103621), let's move forward with active_only and @ts-expect-error (if needed) for now (Client will include the parameter regardless wrong type definition).

@azasypkin
Copy link
Member

ACK: will review today

@azasypkin azasypkin self-requested a review December 20, 2023 12:14
@azasypkin azasypkin added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:Users/Roles/API Keys labels Dec 20, 2023
@elasticmachine
Copy link
Contributor

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

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.

Left a few final notes, but LGTM otherwise! Let's move forward with active_only even if it requires temporary @ts-expect-error.

owner: true,
});

const validKeys = apiResponse.api_keys.filter(({ invalidated }) => !invalidated);
Copy link
Member

Choose a reason for hiding this comment

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

Okay, we'll get this definition in the client soon (elastic/elasticsearch#103621), let's move forward with active_only and @ts-expect-error (if needed) for now (Client will include the parameter regardless wrong type definition).

x-pack/plugins/security/server/routes/api_keys/has.ts Outdated Show resolved Hide resolved
Comment on lines 59 to 61
} catch (error) {
return response.customError(wrapIntoCustomErrorResponse(error));
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: just realized, we don't really need a try/catch here, let's let Core to handle and log this error properly (without exposing too much raw error details).

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 feedback makes sense. After removing the try/catch I also had to delete a unit test, should forward error from Elasticsearch GET API keys endpoint

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #56 / aiops log pattern analysis loads the log pattern analysis page and filters in patterns in discover

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

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, thanks!

@tsullivan tsullivan merged commit b13974c into elastic:main Dec 21, 2023
37 checks passed
@tsullivan tsullivan deleted the security/api-keys-service-has-api-keys branch December 21, 2023 18:33
@kibanamachine kibanamachine added v8.13.0 backport:skip This commit does not require backporting labels Dec 21, 2023
tsullivan added a commit that referenced this pull request Jan 6, 2024
## Summary

This PR offers customizations to the "Getting Started" experience for
users of Serverless Elasticsearch projects. The changes involve checking
Elasticsearch if the currently-logged-in user has created API Keys, and
to use that information to customize the getting started messaging
(destination of the call-to-action of the "no data" prompt) in the UI.

### Screenshots
| | Before | After |
|-|-|-|
| Data Views Mgmt | <img width="1646" alt="before - serverless es data
views mgmt empty state"
src="https://github.com/elastic/kibana/assets/908371/b5c28dda-9ff8-40de-af89-71587bdbbe54">
| <img width="1646" alt="after - serverless es data views mgmt empty
state"
src="https://github.com/elastic/kibana/assets/908371/13aae073-9fef-407a-bdb1-e09d6ea1d168">
|
| Analytics | <img width="1646" alt="before - serverless es discover
empty state"
src="https://github.com/elastic/kibana/assets/908371/ac0e70e5-9a73-4de3-8182-51350099ae87">
| <img width="1646" alt="after - serverless es discover empty state"
src="https://github.com/elastic/kibana/assets/908371/6f9231e0-d2fb-480b-a5a2-79cc9f853a9b">
|

Elasticsearch documentation on API Key listing:
https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-get-api-key.html

Depends on: #172884
Closes: elastic/search-team#5974
Closes: elastic/search-team#5975

### Other changes
* Created a React hook to send and track a request to the Kibana server
to ask whether the user has access to API Keys
    * Added unit test for this
* See discussion, this hook may be [re-organized
eventually](#172225 (comment))
* Enable lazy loading for the "Analytics No Data" page within analytics
apps.

### Checklist

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <[email protected]>
delanni pushed a commit to delanni/kibana that referenced this pull request Jan 11, 2024
## Summary

This PR offers customizations to the "Getting Started" experience for
users of Serverless Elasticsearch projects. The changes involve checking
Elasticsearch if the currently-logged-in user has created API Keys, and
to use that information to customize the getting started messaging
(destination of the call-to-action of the "no data" prompt) in the UI.

### Screenshots
| | Before | After |
|-|-|-|
| Data Views Mgmt | <img width="1646" alt="before - serverless es data
views mgmt empty state"
src="https://github.com/elastic/kibana/assets/908371/b5c28dda-9ff8-40de-af89-71587bdbbe54">
| <img width="1646" alt="after - serverless es data views mgmt empty
state"
src="https://github.com/elastic/kibana/assets/908371/13aae073-9fef-407a-bdb1-e09d6ea1d168">
|
| Analytics | <img width="1646" alt="before - serverless es discover
empty state"
src="https://github.com/elastic/kibana/assets/908371/ac0e70e5-9a73-4de3-8182-51350099ae87">
| <img width="1646" alt="after - serverless es discover empty state"
src="https://github.com/elastic/kibana/assets/908371/6f9231e0-d2fb-480b-a5a2-79cc9f853a9b">
|

Elasticsearch documentation on API Key listing:
https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-get-api-key.html

Depends on: elastic#172884
Closes: elastic/search-team#5974
Closes: elastic/search-team#5975

### Other changes
* Created a React hook to send and track a request to the Kibana server
to ask whether the user has access to API Keys
    * Added unit test for this
* See discussion, this hook may be [re-organized
eventually](elastic#172225 (comment))
* Enable lazy loading for the "Analytics No Data" page within analytics
apps.

### Checklist

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Users/Roles/API Keys release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants