Properly handle password change for users authenticated with provider other than `basic`. #55206

azasypkin · 2020-01-17T17:48:39Z

In this PR we're starting to do a couple of things differently:

We record the type of authentication provider used to authenticated user in the instance of AuthenticatedUser we hand over to the Core in http.registerAuth hook handler.
We switch implementation of getCurrentUser and isAuthenticated to use AuthState created during request authentication stage (yet to be exposed by the core in the scope of Expose Core's Auth State API to the plugins #55011). The methods become synchronous as the result.
We no longer hard code authentication provider type when we re-login user after they change their own password. Now we just use the same provider that was used to authenticate original request.
We no longer try to re-login if user that changed their own password didn't have an active session.
I dropped redundant "stateless" login API that was introduced previously for the related use case.
If user is changing their own password we always send request to Elasticsearch with Authorization: Basic base64(username:current_password) no matter which provider was used to authenticate user. This is required since user must provide a proof of knowledge of the current password.

~~Blocked by: #55011~~
Fixes: #49865

elasticmachine · 2020-01-17T17:48:43Z

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

azasypkin · 2020-02-04T11:15:03Z

x-pack/legacy/plugins/reporting/server/lib/get_user.ts

-      logger.error(err, ['getUser']);
-      return null;
-    }
+    return security?.authc.getCurrentUser(KibanaRequest.from(request)) ?? null;


note: these changes aren't mandatory, just forgot to remove this cleanup. For the majority of places I tried to not change anything.

azasypkin · 2020-02-04T12:07:36Z

x-pack/legacy/plugins/security/index.js

@@ -130,7 +130,7 @@ export const security = kibana =>
        );

      server.expose({
-        getUser: request => securityPlugin.authc.getCurrentUser(KibanaRequest.from(request)),
+        getUser: async request => securityPlugin.authc.getCurrentUser(KibanaRequest.from(request)),


note: for BWC reasons kept legacy API async.

azasypkin · 2020-02-04T12:09:03Z

x-pack/plugins/security/server/authentication/authenticator.ts

@@ -80,13 +80,6 @@ export interface ProviderLoginAttempt {
   * Login attempt can have any form and defined by the specific provider.
   */
  value: unknown;
-


note: reducing API surface. This workaround was introduced exactly for this use case and we don't need it anymore.

azasypkin · 2020-02-04T12:10:11Z

x-pack/plugins/security/server/authentication/providers/base.ts

+      ...(await this.options.client
+        .asScoped({ headers: { ...request.headers, ...authHeaders } })
+        .callAsCurrentUser('shield.authenticate')),
+      // We use `this.constructor` trick to get access to the static `type` field of the specific


note: couldn't find a better way :/

I'm not aware of any other way to get access to a static property from an instance... The only other option that comes to mind is not making getUser return the AuthenticatedUser model, and instead return an interface which doesn't include the authentication_provider field. The authenticator itself could then add the provider which was used, and everything else could continue to use the AuthenticatedUser`.

Yeah, that would be an option too, the tricky thing here is that AuthenticationResult.user that providers return is AuthenticatedUser and we rely on this fact in some other places... I tend to keep what I have for the time being and see how it goes.

azasypkin · 2020-02-04T12:14:25Z

x-pack/plugins/security/server/routes/users/change_password.ts

+      // user with the new password and update session. We check this since it's possible to update
+      // password even if user is authenticated via HTTP headers and hence doesn't have an active
+      // session and in such cases we shouldn't create a new one.
+      if (isUserChangingOwnPassword && currentSession) {


note: we didn't handle the case without session previously, but I think it was a bug. And now we have a proper API that can tell us if we have an active session or not.

…hentication provider.

kibanamachine · 2020-02-04T14:17:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 210cb67

History

💔 Build #24266 failed 516cb45

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

kobelb · 2020-02-04T21:07:50Z

ACK: reviewing

kobelb · 2020-02-04T22:04:05Z

x-pack/plugins/security/server/authentication/providers/base.ts

+      ...(await this.options.client
+        .asScoped({ headers: { ...request.headers, ...authHeaders } })
+        .callAsCurrentUser('shield.authenticate')),
+      // We use `this.constructor` trick to get access to the static `type` field of the specific


I'm not aware of any other way to get access to a static property from an instance... The only other option that comes to mind is not making getUser return the AuthenticatedUser model, and instead return an interface which doesn't include the authentication_provider field. The authenticator itself could then add the provider which was used, and everything else could continue to use the AuthenticatedUser`.

* master: (23 commits) Properly handle password change for users authenticated with provider other than `basic`. (elastic#55206) Improve pull request template proposal (elastic#56756) Only change handlers as the element changes (elastic#56782) [SIEM][Detection Engine] Final final rule changes (elastic#56806) [SIEM][Detection Engine] critical blocker, wrong ilm policy, need to match beats ilm policy Move ui/agg_types in to shim data plugin (elastic#56353) [SIEM] Fixes Signals count spinner (elastic#56797) [docs] Update upgrade version path (elastic#56658) [Canvas] Use unique Id for Canvas Embeddables (elastic#56783) [Rollups] Adjust max width for job detail panel (elastic#56674) Prevent http client from converting our form data (elastic#56772) Disable creating alerts client instances when ESO plugin is using an ephemeral encryption key (elastic#56676) Bumps terser-webpack-plugin to 2.3.4 (elastic#56662) Advanced settings component registry ⇒ kibana platform plugin (elastic#55940) [Endpoint] EMT-67: add kql support for endpoint list (elastic#56328) Implement UI for Create Alert form (elastic#55232) Fix: Filter pill base coloring (elastic#56761) fix open close signal on detail page (elastic#56757) [Search service] Move loadingCount to sync search strategy (elastic#56335) Rollup TSVB integration: Add test and fix warning text (elastic#56639) ...

… other than `basic`. (#55206) (#56831)

azasypkin · 2020-02-05T12:17:53Z

7.x/7.7.0: 20b3db1

… other than `basic`. (elastic#55206)

azasypkin added blocked Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:Security/Authentication Platform Security - Authentication release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Jan 17, 2020

Changes is other plugins.

9eb51bf

azasypkin removed the blocked label Feb 4, 2020

azasypkin force-pushed the issue-xxx-49865-token-passwords branch from 17bd25c to 516cb45 Compare February 4, 2020 11:08

azasypkin commented Feb 4, 2020

View reviewed changes

Properly handle password change for users authenticated via token aut…

210cb67

…hentication provider.

azasypkin force-pushed the issue-xxx-49865-token-passwords branch from 516cb45 to 210cb67 Compare February 4, 2020 12:54

azasypkin marked this pull request as ready for review February 4, 2020 14:23

azasypkin requested a review from a team as a code owner February 4, 2020 14:23

azasypkin requested a review from kobelb February 4, 2020 14:24

kobelb approved these changes Feb 4, 2020

View reviewed changes

azasypkin merged commit 4d7c7b5 into elastic:master Feb 5, 2020

azasypkin deleted the issue-xxx-49865-token-passwords branch February 5, 2020 09:29

azasypkin added the backport pending label Feb 5, 2020

azasypkin mentioned this pull request Feb 5, 2020

[7.x] Properly handle password change for users authenticated with provider other than basic. (#55206) #56831

Merged

azasypkin added a commit that referenced this pull request Feb 5, 2020

Properly handle password change for users authenticated with provider…

20b3db1

… other than `basic`. (#55206) (#56831)

azasypkin added backported and removed backport pending labels Feb 5, 2020

majagrubic pushed a commit to majagrubic/kibana that referenced this pull request Feb 10, 2020

Properly handle password change for users authenticated with provider…

a27fdae

… other than `basic`. (elastic#55206)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly handle password change for users authenticated with provider other than `basic`. #55206

Properly handle password change for users authenticated with provider other than `basic`. #55206

azasypkin commented Jan 17, 2020 •

edited

Loading

elasticmachine commented Jan 17, 2020

azasypkin Feb 4, 2020

azasypkin Feb 4, 2020

azasypkin Feb 4, 2020

azasypkin Feb 4, 2020

kobelb Feb 4, 2020

azasypkin Feb 5, 2020

azasypkin Feb 4, 2020

kibanamachine commented Feb 4, 2020

kobelb commented Feb 4, 2020

kobelb Feb 4, 2020

azasypkin commented Feb 5, 2020

Properly handle password change for users authenticated with provider other than basic. #55206

Properly handle password change for users authenticated with provider other than basic. #55206

Conversation

azasypkin commented Jan 17, 2020 • edited Loading

elasticmachine commented Jan 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Feb 4, 2020

💚 Build Succeeded

History

kobelb commented Feb 4, 2020

Choose a reason for hiding this comment

azasypkin commented Feb 5, 2020

Properly handle password change for users authenticated with provider other than `basic`. #55206

Properly handle password change for users authenticated with provider other than `basic`. #55206

azasypkin commented Jan 17, 2020 •

edited

Loading