[NP] Platform exposes API to get authenticated user data

[mshustov]

Summary

Closes #55011
required for Security plugin to ensure compatibility with the latest ES snapshot #54879
Exposes from the core

• auth.get - returns auth status and associated user data. The platform doesn't enforce user data type at the moment as discussed with @azasypkin. We can re-consider it later.
• auth.isAuthenticated - returns true, if auth interceptor successfully authenticated a user

There are already integration tests for the auth in the place. Probably we should add some for e2e tests, but I want to discuss whether we want to provide this API as a part of the platform at all.
Pros:

• The platform provides auth hook on the server. It means that only the platform knows who uses this hook to authenticate a user and result of this operation.

Cons:

• The platform doesn't enforce the shape of the user data. That reduces type safety for the plugins.
• There is an incompatibility between server and client since the Security plugin is the only source of user data on the client at the moment.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

• [ ] This was checked for cross-browser compatibility, including a check against IE11
• [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• [ ] Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• [ ] This was checked for keyboard-only and screenreader accessibility

For maintainers

• This was checked for breaking API changes and was labeled appropriately
• This includes a feature addition or change that requires a release note and was labeled appropriately

Dev docs

HttpService exposes:

• auth.get() - returns auth status and associated user data. User data are opaque to the http service. Possible auth status values:

  • authenticated - auth interceptor successfully authenticated a user.
  • unauthenticated - auth interceptor failed user authentication.
  • unknown - auth interceptor has not been registered.

• auth.isAuthenticated() - returns true, if auth interceptor successfully authenticated a user.

[elasticmachine]

Pinging @elastic/kibana-platform (Team:Platform)

[azasypkin]

Exposes from the core

auth.get - returns auth status and associated user data. The platform doesn't enforce user data type at the moment as discussed with @azasypkin. We can re-consider it later.
auth.isAuthenticated - returns true, if auth interceptor successfully authenticated a user

That works for the use case we're trying cover, thanks!

Cons:
The platform doesn't enforce the shape of the user data. That reduces type safety for the plugins.

That sounds perfectly fine to me since we can register only one auth hook that's is supposed to fill the auth state, read and interpret it afterwards. Other plugins are supposed to use higher level API exposed by the plugin that owns auth hook handler.

Having said that if we still think that it may become a source of confusion, registerAuth could return "getters" for the state and isAuthenticated so that only code that registers hook can use them. It seems that would also eliminate concerns regarding server vs client inconsistency you mentioned as the second drawback since these "getters" will only be a part server side auth hook API. But either option works for Security plugin.

[mshustov]

Having said that if we still think that it may become a source of confusion, registerAuth could return "getters" for the state and isAuthenticated so that only code that registers hook can use them.

That's another option. My initial implementation was inlined with the idea to make the platform the source of user auth status and user credentials. Security plugin provides API for user authentication to the platform. Plugins (even OSS) use platform API to authenticate user via {route: { auth: 'try'}} or some equivalent of it. In this case, the platform should provide API to retrieve user credentials, probably on both server & client.

Another option would be to keep the current implementation where the Security plugin is the source of auth status/credentials. X-pack plugins access the Security plugin directly as a required dependency to get user credentials. The platform uses injected Security plugin API to perform authentication when required, as discussed in #51438 (comment) With this option your proposal about registerAuth(..) => Auth getters is valid.

[pgayvallet]

(implementation looks fine to me, but more on the actual discussion):

My initial implementation was inlined with the idea was to make the platform the source of user auth status and user credentials

I'm strongly in favor of this decision. We already discussed it, but imho core should have this responsibility. 'security/auth-providers' plugins should only provides the actual implementations for authentication and authorizations.

The platform doesn't enforce the shape of the user data. That reduces type safety for the plugins.

That's true. However in my experience, most pluggable-auth-providers system have the same issue. Usually the principal types share a basic interface to allow to retrieve at least the principal ID without force-casting, but any other access relies on the actual consumer knowing what he's doing.

Would be great if

export type GetAuthState = <T = unknown>(
) => { status: AuthStatus; state: T };

could change to something like:

export type GetAuthState = <T extends {id: string} = {id: string}>(
) => { status: AuthStatus; state?: T };

or

export type GetAuthState = <T extends {getId: () => string} = {getId: () => string}>(
) => { status: AuthStatus; state?: T };

of course that mean changing the security actual implementation to add this accessor and some core types/interfaces such as AuthResultParams and AuthenticationHandler to also requires this base interface in the authentication results/returns

There is an incompatibility between server and client since the Security plugin is the only source of user data on the client at the moment.

I guess if we goes in this direction, we should have a client-side way to retrieve this same data from core APIs. injecting/retrieving the authentication result in the injectedMetadata may do the trick for a basic implementation ?

[azasypkin]

could change to something like:
export type GetAuthState = <T extends {id: string} = {id: string}>(
) => { status: AuthStatus; state?: T };

Out of curiosity, what benefits for plugins from having just opaque principal ID do you envision and what they can use it for (in our case it will be username field that only exists in x-pack world)?

My only concern is that shaping auth state and introducing new client side API that you'll have to maintain BWC for at this stage just for the sake of consistency without clear value proposition may be a bit premature.

[joshdover]

Core being source of truth makes sense 👍. Do we want to expose this on the RequestHandlerContext or would we rather make this harder to find so it's not overused?

could change to something like:
export type GetAuthState = <T extends {id: string} = {id: string}>(
) => { status: AuthStatus; state?: T };

Out of curiosity, what benefits for plugins from having just opaque principal ID do you envision and what they can use it for (in our case it will be username field that only exists in x-pack world)?

My only concern is that shaping auth state and introducing new client side API that you'll have to maintain BWC for at this stage just for the sake of consistency without clear value proposition may be a bit premature.

The only benefit I can think of is if we want to allow users to install custom security plugins and give those plugins the ability to be compatible with our other plugins that need to access this data. I'm not sure that's necessarily something we should do, or even if we should, that we should do it now.

Which parts of the auth data would other plugins besides security/spaces need to access? isAuthenticated is the only I know that would be useful in the try situation. Are there others?

[joshdover]

Should we just expose this on RequestHandlerContext?

[mshustov]

We can expose auth.isAuthenticated when optional authentication implemented. Not sure we need to provide auth.get until then or can be exposed later upon request.

[pgayvallet]

Out of curiosity, what benefits for plugins from having just opaque principal ID do you envision and what they can use it for (in our case it will be username field that only exists in x-pack world)?

May have spoke without thinking. Most common usage of retrieving a user infos from outside of the authent/authoriz plugins/code is to be able to persists data associated with the user, and therefor needing an id to use a technical/func ID. However as every ES call are user-scoped, I guess in our case, the need is not present?

[mshustov]

To recap:

• The platform is becoming the source of auth status & data for the other plugins and provides methods to get user credentials & authenticated status.
• The platform will provide API to perform optional authentication (an equivalent of hapi {auth: 'try'}). I reopened Add an optional authentication mode for HTTP resources #41959
• The platform doesn't provide auth API via RequestHandlerContext until we implement optional authentication.
• The platform doesn't provide the client-side counterpart of auth API until requested.
• X-pack plugins still can use the Security plugin API directly.

Which parts of the auth data would other plugins besides security/spaces need to access?

It's used by X-pack plugins exclusively. What plugins usually access:

• roles.
  https://github.com/elastic/kibana/blob/6dd368139eee15f99f6387dd5a6008c3ddd58981/x-pack/legacy/plugins/dashboard_mode/server/dashboard_mode_request_interceptor.js
  https://github.com/elastic/kibana/blob/6dd368139eee15f99f6387dd5a6008c3ddd58981/x-pack/legacy/plugins/reporting/server/routes/lib/authorized_user_pre_routing.js
• username.
  https://github.com/elastic/kibana/blob/6dd368139eee15f99f6387dd5a6008c3ddd58981/x-pack/legacy/plugins/ml/server/routes/annotations.js#L52

Some of them consume the whole user object, and it doesn't look right.

kibana/x-pack/plugins/case/server/routes/api/post_comment.ts

Line 36 in c6f9eff

createdBy = await caseService.getUser({ request, response });

kibana/x-pack/legacy/plugins/beats_management/server/lib/adapters/framework/kibana_framework_adapter.ts

Line 117 in c6f9eff

...user,

I don't think we consider it as sensitive data, even it contains a full name and email. Since any 3rd party plugin can call security plugin API directly to retrieve the user data. @elastic/kibana-security is it correct?

auth.isAuthenticated can be exposed via RequestHandlerContext when we have optional authentication in place, right now it's not useful as it reflects the route.options.authRequired flag.

isAuthenticated is the only I know that would be useful in the try situation. Are there others?

Nothing I can think of.

@elastic/kibana-platform are you okay with this proposal?

[azasypkin]

The only benefit I can think of is if we want to allow users to install custom security plugins and give those plugins the ability to be compatible with our other plugins that need to access this data. I'm not sure that's necessarily something we should do, or even if we should, that we should do it now.

We definitely want our users to fill the gaps between what we provide and what they may need. And currently the recommended approach to achieve that is to extend Security plugin using the APIs it exposes or may expose in the future. This way users may extend authentication and we'll guarantee that everything will work well with the authorization, feature controls, Spaces etc.

Implementing authentication and authorization is expensive, notoriously hard, error prone and few do this in practice. Bugs in this area may have serious consequences. That's why we don't want to encourage our users to rebuild Security plugin completely and everything that explicitly on implicitly depends on it. On the other hand if users have enough reasons to do so and know what to expect they can do that leveraging the tools Core provides already: auth hook, request interceptors and soon isAuthenticated.

I think having public Core's isAuthenticated makes total sense and aligns well with the rest of Core auth primitives. But I'd still keep authentication state as it's now i.e. completely opaque, no matter how it's retrieved: either via getter in HttpSetup as it's done in the PR or as a return result of onAuth hook. Unless we have a strong reason to change this.

To recap:

LGMT

I don't think we consider it as sensitive data, even it contains a full name and email. Since any 3rd party plugin can call security plugin API directly to retrieve the user data. @elastic/kibana-security is it correct?

Yeah, I don't think there is any sensitive info here, we're exposing this information from Security plugin for a long time.

[kobelb]

I'm jumping into this conversation late, and don't have much to add outside of what is already discussed. I agree with everything Aleh's commented, particularly the following:

I think having public Core's isAuthenticated makes total sense and aligns well with the rest of Core auth primitives. But I'd still keep authentication state as it's now i.e. completely opaque, no matter how it's retrieved: either via getter in HttpSetup as it's done in the PR or as a return result of onAuth hook. Unless we have a strong reason to change this.

[pgayvallet]

To recap:

LGTM

But I'd still keep authentication state as it's now i.e. completely opaque, no matter how it's retrieved: either via getter in HttpSetup as it's done in the PR or as a return result of onAuth hook. Unless we have a strong reason to change this.

I'm ok with that

[joshdover]

Changes look fine, no big changes here really just exposing things.

Should we add a smoke test for this? Can be done as part of #41959

[joshdover]

Why does this keep happening?

[mshustov]

different EOL. fixed in #55689

[pgayvallet]

CRLF to LF conversion. github is very stupid in line terminator diffs

[mshustov]

Should we add a smoke test for this?

@joshdover done in
ab81c4b

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: ab81c4b

History

• 💚 Build #22661 succeeded ab81c4b
• 💚 Build #22076 succeeded 4a41c42
• 💚 Build #21408 succeeded 7d01dd0

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