-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Adding deprecation warning for Interactive Users using ApiKeys #136422
Adding deprecation warning for Interactive Users using ApiKeys #136422
Conversation
Pinging @elastic/kibana-security (Team:Security) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I defer to Oleg for reviewing the implementation, so just a comment about docs from my side. Thanks for picking this up!
expect(routeParamsMock.analyticsService.reportAuthenticationTypeEvent).toHaveBeenCalledWith({ | ||
authenticationProviderType: 'http', | ||
authenticationRealmType: 'native', | ||
httpAuthenticationScheme: 'Custom', | ||
}); | ||
}); | ||
}); | ||
|
||
describe('logApiKeyWithInteractiveUserDeprecated', () => { | ||
it('should log a deprecation warning if interactive user is using API Key', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"interactive user" is not a term I normally see. Can we drop "interactive" or find another way to explain what is meant here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about something like:
should log a deprecation warning if a user is authenticating via API key while accessing the application through a web browser
Co-authored-by: Larry Gregory <[email protected]>
Co-authored-by: Larry Gregory <[email protected]>
Co-authored-by: Larry Gregory <[email protected]>
Co-authored-by: Larry Gregory <[email protected]>
Docs LGTM, pending Oleg's review. Thanks for the edits! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! Just a few minor nits before merging. Also would you mind either updating or removing outdated docs screenshots from the issue description?
x-pack/plugins/security/server/routes/analytics/authentication_type.test.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/routes/analytics/authentication_type.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/routes/analytics/authentication_type.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/routes/analytics/authentication_type.test.ts
Outdated
Show resolved
Hide resolved
…_type.test.ts Co-authored-by: Aleh Zasypkin <[email protected]>
…_type.ts Co-authored-by: Aleh Zasypkin <[email protected]>
…_type.ts Co-authored-by: Aleh Zasypkin <[email protected]>
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
Summary
Resolves #120804
Preview of docs:
Release Notes