Better message for unanticipated authorisation errors #113460

thomheymann · 2021-09-29T17:40:17Z

Resolves #111551

Release note

Add custom error message when receiving 401 response to avoid confusion with Kibana session timeout.

jportner

Looks great so far, glad this turned out to be pretty straightforward!

I have a couple of suggestions below.

jportner · 2021-09-29T17:44:37Z

x-pack/plugins/security/public/authentication/login/login_page.tsx

+    {
+      type: LoginFormMessageType.Info,
+      content: i18n.translate('xpack.security.login.authenticationErrorDescription', {
+        defaultMessage: 'An unexpected authentication error occured. Please log in again.',


Nit:

Suggested change

defaultMessage: 'An unexpected authentication error occured. Please log in again.',

defaultMessage: 'An unexpected authentication error occurred. Please log in again.',

jportner · 2021-09-29T17:46:39Z

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

@@ -32,11 +32,11 @@ const getProviderParameter = (tenant: string) => {
 export class SessionExpired {
  constructor(private logoutUrl: string, private tenant: string) {}

-  logout() {
+  logout(reason = 'SESSION_EXPIRED') {


We are the only consumer of this module --
What do you think about making this argument required so we don't accidentally run into the same problem in the future?

Suggested change

logout(reason = 'SESSION_EXPIRED') {

logout(reason: 'SESSION_EXPIRED' | 'AUTHENTICATION_ERROR') { // see LoginPage component for valid reasons

Agreed but making this argument required isn't going to solve the problem that logout reasons aren't typed and we can't guarantee that we have translated messages for each logout reason. I'll refactor this properly.

…ed-message

jportner

Just for posterity, I tested locally. Here's what I did:

Configure xpack.security.session.idleTImeout: 90s, log in, and allow the session to time out - kicks you back to the login page with the existing session timeout message as expected:
Log in, delete my cookie, and navigate to a different page in Kibana - as soon as an XHR request is made, the client receives a 401 error and kicks you back to the login page with the new message as expected:

One nit below, otherwise LGTM!

jportner · 2021-09-30T14:45:33Z

x-pack/plugins/security/public/authentication/login/components/login_form/login_form.tsx

@@ -513,7 +513,7 @@ export class LoginForm extends Component<Props, State> {
      );

      window.location.href = location;
-    } catch (err) {
+    } catch (err: any) {


Nit: was this change needed? err is already inferred to be of any type, correct?

If we can remove this, we should. We collect stats on explicit usage of any in Kibana and we want to minimize that wherever possible.

Suggested change

} catch (err: any) {

} catch (err) {

typescript complained since err can now be any or undefined so has to be explicitly set to any 🙄

jportner · 2021-09-30T15:02:41Z

x-pack/plugins/security/public/authentication/login/login_page.tsx

-    const query = parse(window.location.href, true).query;
+    const { searchParams } = new URL(window.location.href);
+
    return (
      <LoginForm
        http={this.props.http}
        notifications={this.props.notifications}
        selector={selector}
-        // @ts-expect-error Map.get is ok with getting `undefined`
-        message={messageMap.get(query[LOGOUT_REASON_QUERY_STRING_PARAMETER]?.toString())}
+        message={
+          loginFormMessages[searchParams.get(LOGOUT_REASON_QUERY_STRING_PARAMETER) as LogoutReason]
+        }
        loginAssistanceMessage={this.props.loginAssistanceMessage}
        loginHelp={loginHelp}
-        authProviderHint={query[AUTH_PROVIDER_HINT_QUERY_STRING_PARAMETER]?.toString()}
+        authProviderHint={searchParams.get(AUTH_PROVIDER_HINT_QUERY_STRING_PARAMETER) || undefined}


🙌 🙌 🙌

Oh, @thomheymann I am guessing this change is what caused the async chunks to increase:

Async chunks

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

id before after diff
security 486.8KB 514.4KB +27.6KB

I'm a bit on the fence, we eventually want to get rid of our usage of the url module completely, but maybe we should take this change out until we can change URL parsing in the rest of the plugin (see is_internal_url.ts and parse_next.ts). WDYT?

Actually I don't think using the WHATWG URL API is causing the extra async chunk. It might be the new LogoutReason enum added to the common module. I'll do some experimenting to see

That's strange, if anything I'd expect the bundle size to go down since we're not including url module anymore.

jportner · 2021-09-30T16:03:57Z

@elastic/kibana-operations I'm trying to figure out what is causing the async chunk size to increase so drastically.

From the build metrics above:

I am not sure what could be causing this.

we removed one import of the url module
we added a couple imports of the x-pack/security/common/types module (which is quite small, it just contains some types and an enum

This seems to have caused a 27K increase in total async chunks.

Referencing the Keep Kibana fast page in the docs, I ran this command to get the metrics with and without Thom's changes:

node scripts/build_kibana_platform_plugins.js --dist --profile --focus=security

I've attached them here:
stats.json-files.zip

However, I can't make heads or tails of this when trying to compare before/after. It appears that the chunks have been reordered have lots of different contents (see security.chunk.2.js from before vs security.chunk.1.js after, for example).

Thoughts? Is there anything obvious we can do to prevent this 27K increase in async chunk size?

jportner · 2021-10-01T20:13:10Z

However, I can't make heads or tails of this when trying to compare before/after. It appears that the chunks have been reordered have lots of different contents (see security.chunk.2.js from before vs security.chunk.1.js after, for example).

Thoughts? Is there anything obvious we can do to prevent this 27K increase in async chunk size?

I did a bit more digging on this, it doesn't seem like there is an easy fix we can implement today. It may involve extensive refactoring. I opened a separate ER for this: #113670

In the meantime I think we can merge this PR and move on with our lives 👍

jportner · 2021-10-01T20:13:30Z

@elasticmachine merge upstream

jportner · 2021-10-04T12:46:39Z

@elasticmachine merge upstream

kibanamachine · 2021-10-04T14:51:55Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`security`	455	456	+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`	487.0KB	514.6KB	+27.6KB

Page load bundle

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

id	before	after	diff
`security`	49.2KB	49.4KB	+231.0B

Unknown metric groups

async chunk count

id	before	after	diff
`security`	23	21	-2

History

💚 Build #157233 succeeded 55225e0
💛 Build #156688 was flaky fab0034
💔 Build #156659 failed a60d9da
💔 Build #156540 failed 19be40b

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

spalger · 2021-10-04T17:14:29Z

@elastic/kibana-operations I'm trying to figure out what is causing the async chunk size to increase so drastically.

I wouldn't worry about it too much. The async chunks are calculated to reduce the amount of load operations and can sometimes include duplicated code so that one chunk can load instead of another. The changes in the import graph are sometimes impossible for us to understand and the total "async chunk size" only represents the size of assets that would be loaded if every async load was deemed necessary (which is highly unlikely).

* Custom message for unanticipated 401 errors * Refactor logout reasons * Fix types Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-10-04T17:21:37Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

) * Custom message for unanticipated 401 errors * Refactor logout reasons * Fix types Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Thom Heymann <190132+thomheymann@users.noreply.github.com>

Custom message for unanticipated 401 errors

19be40b

thomheymann added release_note:fix Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.0.0 auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Sep 29, 2021

thomheymann requested a review from a team as a code owner September 29, 2021 17:40

jportner reviewed Sep 29, 2021

View reviewed changes

thomheymann added 3 commits September 30, 2021 09:41

Refactor logout reasons

03f88b2

Merge branch 'master' of github.com:elastic/kibana into session-expir…

a60d9da

…ed-message

Fix types

fab0034

thomheymann requested a review from jportner September 30, 2021 12:09

jportner approved these changes Sep 30, 2021

View reviewed changes

jportner self-requested a review September 30, 2021 15:12

jportner mentioned this pull request Oct 1, 2021

Reduce Security plugin's async chunk size (static assets) #113670

Closed

jportner approved these changes Oct 1, 2021

View reviewed changes

Merge branch 'master' into session-expired-message

55225e0

Merge branch 'master' into session-expired-message

9ab3bc5

thomheymann merged commit 6937276 into elastic:master Oct 4, 2021

kibanamachine mentioned this pull request Oct 4, 2021

[7.x] Better message for unanticipated authorisation errors (#113460) #113806

Merged

legrego mentioned this pull request Jan 25, 2022

Improve handling of 401 responses #110264

Closed

jportner mentioned this pull request Feb 1, 2022

[Discuss] [Kibana platform security] Session tracking and audit logging #91285

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better message for unanticipated authorisation errors #113460

Better message for unanticipated authorisation errors #113460

thomheymann commented Sep 29, 2021

jportner left a comment

jportner Sep 29, 2021

jportner Sep 29, 2021

thomheymann Sep 30, 2021

jportner left a comment

jportner Sep 30, 2021

thomheymann Sep 30, 2021

jportner Sep 30, 2021

jportner Sep 30, 2021

jportner Sep 30, 2021

thomheymann Sep 30, 2021

jportner commented Sep 30, 2021

jportner commented Oct 1, 2021

jportner commented Oct 1, 2021

jportner commented Oct 4, 2021

kibanamachine commented Oct 4, 2021

async chunk count

spalger commented Oct 4, 2021

kibanamachine commented Oct 4, 2021

	defaultMessage: 'An unexpected authentication error occured. Please log in again.',
	defaultMessage: 'An unexpected authentication error occurred. Please log in again.',

	logout(reason = 'SESSION_EXPIRED') {
	logout(reason: 'SESSION_EXPIRED' \| 'AUTHENTICATION_ERROR') { // see LoginPage component for valid reasons

Better message for unanticipated authorisation errors #113460

Better message for unanticipated authorisation errors #113460

Conversation

thomheymann commented Sep 29, 2021

Release note

jportner 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

jportner 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

Async chunks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jportner commented Sep 30, 2021

jportner commented Oct 1, 2021

jportner commented Oct 1, 2021

jportner commented Oct 4, 2021

kibanamachine commented Oct 4, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

History

spalger commented Oct 4, 2021

kibanamachine commented Oct 4, 2021

💚 Backport successful