Skip to content
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

[Security] Add message to login page #51557

Merged
merged 7 commits into from
Nov 26, 2019

Conversation

sorenlouv
Copy link
Member

@sorenlouv sorenlouv commented Nov 24, 2019

This change makes it possible to add an optional message to the login page. This is useful for announcing scheduled maintenance, and providing help with credentials - something we currently need the ability to do for observability.

Example 1: Maintenance announcements

Example 2: Help with credentials (corporate)

Example 3: Help with credentials (test cluster)

@sorenlouv sorenlouv requested review from a team as code owners November 24, 2019 21:01
@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv sorenlouv added the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label Nov 25, 2019
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@watson
Copy link
Contributor

watson commented Nov 25, 2019

For scheduled maintenance announcements, I think it would be better to have a banner that also appears after login and not only during login. A user can be logged in for a long time and not see the login screen. So I'm not sure about how big a benefit this will be only on the login screen.

However, I like the idea of having a message like this related to the actual login procedure.

@sorenlouv
Copy link
Member Author

sorenlouv commented Nov 25, 2019

@watson True, maintenance announcements might not be the best use case. Help to login is specifically the problem we are trying to solve. For the Kibana instances I'm responsible for I'm regularly pinged by people who want to know how they can login. For this specific purpose it would be very beneficial to be able to add login instructions (or link to such) directly above the login box.

I'm sure there are also other use cases people might want to use this message for so tried to keep it a little generic.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jbudz
Copy link
Member

jbudz commented Nov 25, 2019

We do have
image too, only after login but could be reused for a global. I do think there's utility with a server defined message like this too

@legrego
Copy link
Member

legrego commented Nov 25, 2019

@alexfrancoeur would this satisfy #36586?

I think there is a small part of #17298 in here too, but that is mostly concerned with post-login messages

@sorenlouv
Copy link
Member Author

sorenlouv commented Nov 25, 2019

More generally, the ability to add a custom html site link to login page for admin user to specify text of the link and link to external page through the management interface in Kibana.
#36586 (comment)

Users cannot use custom html but they can use markdown, so links and formatting is possible (and I'd argue safer and easier than allowing custom html).

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the "logout message" prevents the new login assistance message from rendering:

image

I'd expect both messages to appear, but they're both using the primary EuiCallOut color, so I worry one might get lost next to the other. This might be ok, but maybe @elastic/kibana-design has an opinion?

image

@legrego
Copy link
Member

legrego commented Nov 25, 2019

Users cannot use custom html but they can use markdown, so links and formatting is possible (and I'd argue safer and easier than allowing custom html).

++ I'd be wary of custom HTML too, and I think markdown allows for a sufficient level of control here.

@legrego legrego added the v8.0.0 label Nov 25, 2019
@cchaos
Copy link
Contributor

cchaos commented Nov 25, 2019

I would not put the custom message inside of an EuiCallOut. Rendering can be tricky with custom content (even just Markdown) as seen by @legrego's example. Though if it is just plain Markdown, it should get wrapped in an EuiText (size="s") block.

@sorenlouv
Copy link
Member Author

sorenlouv commented Nov 25, 2019

@cchaos Better?

image

@sorenlouv
Copy link
Member Author

sorenlouv commented Nov 25, 2019

It looks like the "logout message" prevents the new login assistance message from rendering:

@legrego Good point. Since c229c4c it now renders the login message independently of the other messages.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sqren Yes, I think it's fair to say that not all custom messages time or content sensitive. Which is why pulling it out of the EuiCallOut makes the most sense. If it is, they can add their own emphasis via text formatting.

Co-Authored-By: Caroline Horn <[email protected]>
@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv
Copy link
Member Author

I would assume we'd want our callout though to be closest to the login form (just above the panel).

Okay, I'll flip them around.

@sorenlouv
Copy link
Member Author

image

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from my end. 👍

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@watson watson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@sorenlouv
Copy link
Member Author

I'll merge this for now to unblock some things. LMK if I should change the name xpack.security.loginAssistanceMessage to something else (naming is hard) and I can do that in a follow-up PR.

@sorenlouv sorenlouv merged commit e8e5174 into elastic:master Nov 26, 2019
@sorenlouv sorenlouv deleted the add-login-assistance-message branch November 26, 2019 12:19
sorenlouv added a commit to sorenlouv/kibana that referenced this pull request Nov 26, 2019
* [Security] Add loginAssistanceMessage to login page

* Fix tests

* Fix login_page.test.tsx

* Fix defaultValue

* Render login assistance message independently of other messages and use EuiText instead of EuiCallOut

* Use small text

Co-Authored-By: Caroline Horn <[email protected]>

* Flip order of message around
sorenlouv added a commit that referenced this pull request Nov 26, 2019
* [Security] Add loginAssistanceMessage to login page

* Fix tests

* Fix login_page.test.tsx

* Fix defaultValue

* Render login assistance message independently of other messages and use EuiText instead of EuiCallOut

* Use small text

Co-Authored-By: Caroline Horn <[email protected]>

* Flip order of message around
@kresss
Copy link

kresss commented Jan 6, 2020

Do users still see this new message if they are logging into a PKI protected kibana? Ideally they should have to acknowledge the banner before being logged in even if they are not providing username/password style credentials.

@kobelb
Copy link
Contributor

kobelb commented Jan 7, 2020

Do users still see this new message if they are logging into a PKI protected kibana? Ideally they should have to acknowledge the banner before being logged in even if they are not providing username/password style credentials.

They do not. This message is only shown on the login page, which only works with the Reserved/Native/File realms in Elasticsearch.

Would you mind chiming in on either #18176 and/or #17298 with your specific needs?

@kresss
Copy link

kresss commented Jan 7, 2020

Would you mind chiming in on either #18176 and/or #17298 with your specific needs?

@kobelb I think that our requirements are stated in #18176. The comment from @mbarretta restates the requirement.

@kobelb
Copy link
Contributor

kobelb commented Jan 7, 2020

Thanks @kresss

@cauemarcondes
Copy link
Contributor

Tests:
IE: ✅
Chrome: ✅
Firefox: ✅
Safari: ✅
Screenshot 2020-01-20 at 12 03 22

@cauemarcondes cauemarcondes added the apm:test-plan-done Pull request that was successfully tested during the test plan label Jan 20, 2020
@watson watson mentioned this pull request Jan 27, 2020
@azasypkin
Copy link
Member

6.8/6.8.9: 2baa83b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:test-plan-done Pull request that was successfully tested during the test plan backported release_note:enhancement Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v6.8.9 v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.