Make all providers to preserve original URL when session expires. #84229

azasypkin · 2020-11-24T15:10:32Z

Summary

This PR makes SAML/OIDC/PKI/Kerberos/Anonymous providers to remember current URL when session expires and user is redirected to the Login Selector or Logged Out pages. With this when user logs in again they will end up at the same page they were when the session expired.

Notable exception: if SAML SLO is enabled then during logout user will be redirected to the IdP and the current URL will be lost. We may tackle this in the scope of #69506 since we'll likely be creating an unauthenticated session during logout to store SAML Logout Request ID and hence may store current URL as well.

Release note

Previously when SAML/OIDC/PKI/Kerberos session expired users were logged out and redirected to the login screen to re-login losing their original URL. With this change users will be now redirected back to the original URL after they log in again.

Fixes: #70398

azasypkin · 2020-11-26T15:38:35Z

x-pack/plugins/security/common/constants.ts

@@ -19,3 +19,5 @@ export const APPLICATION_PREFIX = 'kibana-';
 export const RESERVED_PRIVILEGES_APPLICATION_WILDCARD = 'kibana-*';

 export const AUTH_PROVIDER_HINT_QUERY_STRING_PARAMETER = 'auth_provider_hint';
+export const LOGOUT_REASON_QUERY_STRING_PARAMETER = 'msg';


note: I'm not a fan of moving params to the consts since it usually makes it harder to find the code that uses them. But in this specific case these parameter names are so generic that it's much easier to rely on the unique const names to find all relevant places.

azasypkin · 2020-11-26T15:41:04Z

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

+    const suggestedProviderName =
+      sessionValue?.provider.name ??
+      request.url.searchParams.get(AUTH_PROVIDER_HINT_QUERY_STRING_PARAMETER);
+    if (suggestedProviderName) {
      await this.session.clear(request);


note: it's a no-op in case session doesn't exist

azasypkin · 2020-11-30T07:54:51Z

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

+    // logout reason that login page may need to know.
+    return this.options.config.authc.selector.enabled || shouldProviderUseLoginForm(providerType)
+      ? `${this.options.basePath.serverBasePath}/login?${searchParams.toString()}`
+      : `${this.options.basePath.serverBasePath}/security/logged_out?${searchParams.toString()}`;


note: logged_out should support not only next, but also msg to eventually display different error messages for the normal logout and the logout caused by the session timeout #84200.

elasticmachine · 2020-11-30T08:30:27Z

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

legrego

Tested with multiple providers, multiple spaces, and the access agreement interstitial screen -- all seem to be working perfectly! The fact that this works with our hash-based routes too is 🥇

legrego · 2020-12-01T11:58:14Z

x-pack/plugins/security/common/types.ts

-/**
- * Type and name tuple to identify provider used to authenticate user.
- */
-export interface AuthenticationProvider {


question I'm 100% fine with the move, but I'm just curious: what was the motivation for moving this interface out of types and into its own file?

Mostly because I wanted to group interface with the relevant helper function shouldProviderUseLoginForm (like we do for the AuthenticatedUser) and having function in types.ts would look a bit weird.

legrego · 2020-12-01T12:09:31Z

x-pack/plugins/security/server/audit/security_audit_logger.ts

@@ -4,7 +4,7 @@
 * you may not use this file except in compliance with the Elastic License.
 */

-import { AuthenticationProvider } from '../../common/types';
+import type { AuthenticationProvider } from '../../common/model';


question: you've been great about updating these import statements to import type. Is this a hint that WebStorm provides, or are you just naturally more observant than I am? 😄

Haha, nope, WebStorm doesn't suggest that change yet. I think I just shuffled import's so many times that now I know where I can add that type 🙈

legrego · 2020-12-01T17:29:39Z

x-pack/plugins/security/public/session/session_expired.ts

 };

 const getProviderParameter = (tenant: string) => {
  const key = `${tenant}/session_provider`;
  const providerName = sessionStorage.getItem(key);
-  return providerName ? `&provider=${encodeURIComponent(providerName)}` : '';
+  return providerName
+    ? `&${AUTH_PROVIDER_HINT_QUERY_STRING_PARAMETER}=${encodeURIComponent(providerName)}`


note this change initially confused me. I thought this was going to end up automatically logging the user back in for providers that don't require user interaction (anonymous, pki, kerberos, etc). Once I traced through the code, I understood that this was instead being used to inform the logout routine about which provider should handle the logout.

I'm ok with this change, but now we have two distinct uses for auth_provider_hint: One is meant for public consumption, and will become part of our public API, and this new case is more if an implementation detail that's unrelated to the public API.

Was consistency the motivation for this change, or was there another reason we changed the parameter name?

Was consistency the motivation for this change, or was there another reason we changed the parameter name?

Yeah, just wanted to use less query string parameters and decided to re-use this one since its name isn't strictly bound to its current purpose yet and is more or less relevant to both cases. I don't have a strong on this though, if it feels confusing to you (and I see where the confusion is coming from) I can switch back to provider (or something even more descriptive). What do you think?

I have a slight preference for provider over auth_provider_hint, but I don't feel strongly enough to have you change it. Happy for you to merge as-is

Good 👍 I'll reverted back to provider. If it confused you then it can confuse someone else who reads this code in the future too

…d during logout.

kibanamachine · 2020-12-02T10:29:55Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8d885ec

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`security`	471	472	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`security`	787.1KB	786.1KB	-1.0KB

Distributable file count

id	before	after	diff
`default`	43200	43201	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`security`	163.5KB	164.6KB	+1.1KB

History

💚 Build #90589 succeeded 1d8f121
💚 Build #90488 succeeded 87a7f75
💔 Build #90460 failed 78b1508
💔 Build #90451 failed 5e9dce1
💔 Build #89978 failed 70a2558

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

…astic#84229) # Conflicts: # x-pack/plugins/security/server/authentication/authenticator.ts

azasypkin · 2020-12-02T12:55:49Z

7.x/7.11.0: 4b7e868

* master: (72 commits) Make alert status fetching more resilient (elastic#84676) [APM] Refactor hooks and context (elastic#84615) Added word break styles to the texts in the item details card. (elastic#84654) [Search] Disable "send to background" when auto-refresh is enabled (elastic#84106) Add readme for new palette service (elastic#84512) Make all providers to preserve original URL when session expires. (elastic#84229) [Lens] Show color in flyout instead of auto (elastic#84532) [Lens] Use index pattern through service instead of reading saved object (elastic#84432) Make it possible to use Kibana anonymous authentication provider with ES anonymous access. (elastic#84074) TelemetryCollectionManager: Use X-Pack strategy as an OSS overwrite (elastic#84477) migrate away from rest_total_hits_as_int (elastic#84508) [Input Control] Custom renderer (elastic#84423) Attempt to more granularly separate App Search vs Workplace Search vs shared GitHub notifications (elastic#84713) [Security Solutino][Case] Case connector alert UI (elastic#82405) [Maps] Support runtime fields in tooltips (elastic#84377) [CCR] Fix row actions in follower index and auto-follow pattern tables (elastic#84433) [Enterprise Search] Migrate shared Indexing Status component (elastic#84571) [maps] remove fields from index-pattern test artifacts (elastic#84379) Add routes for use in Sources Schema (elastic#84579) Changes UI links for drilldowns (elastic#83971) ...

azasypkin added release_note:enhancement Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:Security/Authentication Platform Security - Authentication v7.11.0 labels Nov 24, 2020

azasypkin force-pushed the issue-70398-preserve-url-during-logout branch from 70a2558 to 5e9dce1 Compare November 26, 2020 15:35

azasypkin added the v8.0.0 label Nov 26, 2020

azasypkin commented Nov 26, 2020

View reviewed changes

azasypkin force-pushed the issue-70398-preserve-url-during-logout branch 2 times, most recently from 78b1508 to 87a7f75 Compare November 26, 2020 18:22

Make all providers to preserve original URL when session expires.

1d8f121

azasypkin force-pushed the issue-70398-preserve-url-during-logout branch from 87a7f75 to 1d8f121 Compare November 30, 2020 06:42

azasypkin commented Nov 30, 2020

View reviewed changes

azasypkin marked this pull request as ready for review November 30, 2020 08:30

azasypkin requested a review from a team as a code owner November 30, 2020 08:30

legrego self-requested a review November 30, 2020 16:35

legrego approved these changes Dec 1, 2020

View reviewed changes

azasypkin added 2 commits December 2, 2020 09:36

Merge branch 'master' into issue-70398-preserve-url-during-logout

2cda9ad

Review#1: revert back the name of provider query string parameter use…

8d885ec

…d during logout.

azasypkin merged commit 59a405d into elastic:master Dec 2, 2020

azasypkin deleted the issue-70398-preserve-url-during-logout branch December 2, 2020 10:32

azasypkin mentioned this pull request Dec 2, 2020

[7.x] Make all providers to preserve original URL when session expires. (#84229) #84743

Merged

azasypkin added a commit to azasypkin/kibana that referenced this pull request Dec 2, 2020

Make all providers to preserve original URL when session expires. (el…

777e51b

…astic#84229) # Conflicts: # x-pack/plugins/security/server/authentication/authenticator.ts

azasypkin added the backported label Dec 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make all providers to preserve original URL when session expires. #84229

Make all providers to preserve original URL when session expires. #84229

azasypkin commented Nov 24, 2020 •

edited

Loading

azasypkin Nov 26, 2020

azasypkin Nov 26, 2020

azasypkin Nov 30, 2020

elasticmachine commented Nov 30, 2020

legrego left a comment

legrego Dec 1, 2020

azasypkin Dec 1, 2020

legrego Dec 1, 2020

azasypkin Dec 1, 2020

legrego Dec 1, 2020

azasypkin Dec 1, 2020

legrego Dec 1, 2020

azasypkin Dec 2, 2020

kibanamachine commented Dec 2, 2020

azasypkin commented Dec 2, 2020

Make all providers to preserve original URL when session expires. #84229

Make all providers to preserve original URL when session expires. #84229

Conversation

azasypkin commented Nov 24, 2020 • edited Loading

Summary

Release note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Nov 30, 2020

legrego left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Dec 2, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Distributable file count

Page load bundle

History

azasypkin commented Dec 2, 2020

azasypkin commented Nov 24, 2020 •

edited

Loading