Redesigned user profile page #127624

thomheymann · 2022-03-14T16:51:22Z

Redesigned user profile page.

Profile page (ordinary user)

Profile page (ordinary user, profile picture)

Profile page (ordinary user, change password modal)

Profile page (ordinary user with minimal privileges)

Profile page (reserved user)

Profile page (reserved user authenticated via authenticating proxy)

Note: such users cannot have profiles, but they still should have access to the profile page

Profile page (ordinary user authenticated via authenticating proxy)

Note: such users cannot have profiles, but they still should have access to the profile page

Previous revision

Form changes

Error state

Reserved user

Get profile API

http://localhost:5601/internal/security/user_profile?data=*

{
  "uid": "u_7Nv1ik1uRbakhv8uXHrVpQ",
  "enabled": true,
  "user": {
    "username": "thom",
    "roles": [
      "superuser",
      "can_manage_profile"
    ],
    "realm_name": "default_native",
    "email": "[email protected]",
    "full_name": "Thomas Heymann",
    "active": true
  },
  "data": {
    "advanced_settings": "foo",
    "security_solution": "bar",
    "avatar": {
      "color": "#DDAFEF",
      "initials": "TH",
      "imageUrl": null
    }
  },
  "authentication_provider": {
    "type": "basic",
    "name": "basic"
  }
}

Testing

Reserved (superuser): log in with elastic:changeme
Native (superuser): create a new user with the superuser role, and log in
Native (minimal privileges): create a user with the viewer role, and log in
Sessionless: use a reverse proxy to inject an HTTP authorization header in each request

Click to see reverse proxy code

const http = require('http');

const USERNAME = 'elastic';
const PASSWORD = 'changeme';
const AUTHZ_HEADER = `Basic ${Buffer.from(`${USERNAME}:${PASSWORD}`).toString('base64')}`
console.log(`Using credentials "${USERNAME}:${PASSWORD}" (Authorization: ${AUTHZ_HEADER})`);

function onRequest(req, res) {
  console.log('serve: ' + req.url);

  const options = {
    hostname: 'localhost',
    port: 5601,
    path: req.url,
    method: req.method,
    headers: {
      ...req.headers,
      'Authorization': AUTHZ_HEADER
    }
  };

  const proxy = http.request(options, function (r) {
    res.writeHead(r.statusCode, r.headers);
    r.pipe(res, {
      end: true
    });
  });

  req.pipe(proxy, {
    end: true
  });
}

http.createServer(onRequest).listen(3000);
console.log('Listening on port 3000')

thomheymann

Self review

package.json

x-pack/plugins/security/public/account_management/user_profile/user_profile.tsx

x-pack/plugins/security/public/account_management/user_profile/form_field.tsx

x-pack/plugins/security/public/account_management/user_profile/user_profile.tsx

x-pack/plugins/security/public/account_management/index.ts

x-pack/plugins/security/server/routes/user_profile/get.ts

x-pack/plugins/security/public/nav_control/nav_control_component.tsx

azasypkin · 2022-03-16T09:13:04Z

ACK: reviewing...

azasypkin

Looks good! I didn't spend too much time testing, but will do that during the final review.

I noticed that anonymous users aren't supported yet, is it expected at this point (profile spinner is stuck and I see 404 for http://localhost:5601/oleg/internal/security/user_profile?data=avatar request)?

x-pack/plugins/security/common/model/user_profile.ts

x-pack/plugins/security/public/account_management/index.ts

x-pack/plugins/security/public/components/use_current_user.ts

x-pack/plugins/security/public/account_management/user_profile/user_avatar.tsx

x-pack/plugins/security/public/account_management/user_profile/form_row.tsx

x-pack/plugins/security/public/account_management/user_profile/form_field.tsx

x-pack/plugins/security/public/account_management/user_profile/user_profile.tsx

jportner

High level review--

Looking great so far, I think the overall structure makes sense. I noticed a couple of tiny issues:

Form validation - typing a new password that's too short causes the validation text to show up twice, which looks weird. I'm not too fussed about it (I think it already behaved this way?), just thought I'd point it out:
Changing "Full name" will also change the initials. And if you change the initials, discard the changes, and change the full name again, it still works. But if you change the full name (Joe Portner -> Joe Diggity), then change the initials back to what they started out as (JD -> JP), then change the full name again, the initials won't change. If you discard changes after this, the initials will still never change when modifying the full name, the only thing that seems to fully reset it is a page reload. Something funky with state management I guess, not a big deal but again I thought I'd point it out in case you think it's worth investigating.
I can't update my profile, I get a 403 error, I guess that's expected though right now, correct?

jportner · 2022-03-21T22:48:07Z

yarn.lock

@@ -19910,6 +19923,11 @@ lodash-es@^4.17.11:
  resolved "https://registry.yarnpkg.com/lodash-es/-/lodash-es-4.17.15.tgz#21bd96839354412f23d7a10340e5eac6ee455d78"
  integrity sha512-rlrc3yU3+JNOpZ9zj5pQtxnx2THmvRykwL4Xlxoa8I9lHBlVbbyPhgyPMioxVZ4NqyxaVVtaJnzsyOidQIhyyQ==

+lodash-es@^4.17.21:


Nit: you could collapse this dependency with lodash-es@^4.17.11 above

thomheymann · 2022-05-19T20:27:06Z

Validation appears to happen on blur of the "Initials" input. Would it be possible to change the validation checks to occur when the user clicks the "Save changes" button instead? I think that might be less annoying.

The validation currently happens on blur and on change. We definitely can perform validation only once user click the "Save changes", but what about the case when user tries to correct invalid values after validation? Should we keep "invalid" state/message until user clicks on the "Save changes" button again or the form should behave differently in this case (e.g. re-validate as user types)?

Excellent question! Ideally, it would be great if we could perform the first validation only when the user clicks the "Save changes" button. If one or more of the form inputs is invalid, validation error states/messages should appear on those inputs. At this point, the validation model could change to on blur/change (as the user's mindset has changed from completing the form to correcting the form). Would that be possible?

@MichaelMarcialis What's the reasoning for changing the logic here to show inline validation errors only after the user clicked submit? The purpose of inline validation is to show errors in context after the user changes a field so that they get immediate feedback and don't have to go back to previously fields that were invalid after clicking submit. This change would also be inconsistent with other forms in Kibana. I think it's important to have a consistent approach here.

azasypkin · 2022-05-20T13:48:14Z

P.S. In this PR we don't disable avatar editing for the ESS SAML users, I'm working on this in a separate PR.

PR for that is ready 9b2b3ea (based on and blocked by this PR)

jportner

Looking good!

In addition to my inline comments below:

I suggest moving the two ContextProvider/constate files out of the public/components directory and somewhere else (maybe public/user_profile_context?)
I noticed that the Edit User page doesn't show the correct avatar; I opened a separate issue for us to address that later (Support for viewing another user's avatar #132636)

x-pack/plugins/security/public/account_management/user_profile/user_avatar.tsx

x-pack/plugins/security/public/components/use_current_user.ts

x-pack/plugins/security/public/components/api_clients_provider.ts

x-pack/plugins/security/public/components/use_current_user.ts

x-pack/plugins/security/server/user_profile/user_profile_service.mock.ts

jportner

The changes I requested can be made at a later time, I'll go ahead and approve the PR

Edit by Larry: Followup issue to address this feedback: #132645

gchaps · 2022-05-20T19:11:16Z

How about using a more specific title for the message:

Title: Kibana will lose connection to Elasticsearch
Description: After changing the password for the kibana_system user, your Elasticsearch data won't be available in Kibana. To regain access, update your config file with the new password and restart Kibana.

jportner · 2022-05-20T20:45:23Z

How about using a more specific title for the message:

Title: Kibana will lose connection to Elasticsearch Description: After changing the password for the kibana_system user, your Elasticsearch data won't be available in Kibana. To regain access, update your config file with the new password and restart Kibana.

I like the more specific title.
The dsecription might be a bit misleading though --
When this happens, Kibana will be completely unable to communicate with Elasticsearch.
It's not just that you won't be able to see your Elasticsearch data, you won't be able to log into Kibana or even use it at all.

How about this for a description?

After changing the password for the kibana_system user, Kibana will be unusable. To regain access, update your config file with the new password and restart Kibana.

legrego · 2022-05-22T17:04:08Z

How about using a more specific title for the message:
Title: Kibana will lose connection to Elasticsearch Description: After changing the password for the kibana_system user, your Elasticsearch data won't be available in Kibana. To regain access, update your config file with the new password and restart Kibana.

I like the more specific title. The dsecription might be a bit misleading though -- When this happens, Kibana will be completely unable to communicate with Elasticsearch. It's not just that you won't be able to see your Elasticsearch data, you won't be able to log into Kibana or even use it at all.

How about this for a description?

After changing the password for the kibana_system user, Kibana will be unusable. To regain access, update your config file with the new password and restart Kibana.

I added this feedback to #132645 for us to address in a followup

…hanging password for Kibana system users.

kibana-ci · 2022-05-23T09:00:35Z

💔 Build Failed

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #38 / Actions and Triggers app Rule status filter should allow rule statuses to be filtered

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [169d503]

History

💔 Build #46144 failed 3e282fe
💔 Build #46076 failed a887dad
💔 Build #45638 failed cf1c0ce
💔 Build #45356 failed accd6c0
💔 Build #45312 failed a1b53ef
💔 Build #43107 failed 1ece49a

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

cc @MichaelMarcialis

x-pack/plugins/security/public/management/users/edit_user/change_password_modal.tsx

MichaelMarcialis · 2022-05-23T15:11:08Z

x-pack/plugins/security/public/components/form_label.tsx

+    <EuiFlexGroup responsive={false} gutterSize="xs">
+      <EuiFlexItem grow={false}>{props.children}</EuiFlexItem>
+      {!isEqual ? (
+        <EuiFlexItem grow={false}>
+          <EuiIcon type="dot" color="success" />
+        </EuiFlexItem>
+      ) : undefined}
+    </EuiFlexGroup>


Ah, yes. I see the 1px jump now when the icon is added and removed with my suggestion. I believe this can be corrected by applying a style of vertical-align: text-top; to the icon component.

MichaelMarcialis

Amazing! Thank you so much for addressing all of my prior feedback, @azasypkin! This is looking excellent. I'm leaving you a few small comments for the re-review.

When a user's "Full name" and "Email address" are immutable (i.e. user doesn't have permission to change on profile page), they currently aren't being shown in the header when not populated. Can we instead change this to always show those immutable fields in the header with a "None provided" message, as shown in designs?

It looks like there is no left/right padding being applied to the page template (most noticeable at small viewport widths). I believe the page templates should come with such padding baked-in by default (see EUI docs; we should also be using these props to apply the page max-width I imagine). Would it be possible to restore that padding here?

For the unsaved changes EuiBottomBar contents, would it be possible to add a responsive = {false} prop to the contained EuiFlexGroup. Doing so would help prevent it from looking odd at small viewport sizes. If there's concerns about too little horizontal space, we can also remove/hide the EuiFlexItem that contains the unsaved changes count size to gain back some space.

I noticed that the unsaved changes EuiBottomBar can fall outside of viewport (noticeable at small viewport heights). I assume this is happening because the position prop has been set to sticky. Can we correct this so that the EuiBottomBar uses the default fixed positioning to the bottom of the viewport (as it is in Advanced Settings)?

Assuming these comments can be addressed, I'll go ahead and approve this now so I don't hold you up further for feature freeze (as these are relatively small issues).

MichaelMarcialis · 2022-05-23T17:44:32Z

Validation appears to happen on blur of the "Initials" input. Would it be possible to change the validation checks to occur when the user clicks the "Save changes" button instead? I think that might be less annoying.

The validation currently happens on blur and on change. We definitely can perform validation only once user click the "Save changes", but what about the case when user tries to correct invalid values after validation? Should we keep "invalid" state/message until user clicks on the "Save changes" button again or the form should behave differently in this case (e.g. re-validate as user types)?

Excellent question! Ideally, it would be great if we could perform the first validation only when the user clicks the "Save changes" button. If one or more of the form inputs is invalid, validation error states/messages should appear on those inputs. At this point, the validation model could change to on blur/change (as the user's mindset has changed from completing the form to correcting the form). Would that be possible?

@MichaelMarcialis What's the reasoning for changing the logic here to show inline validation errors only after the user clicked submit? The purpose of inline validation is to show errors in context after the user changes a field so that they get immediate feedback and don't have to go back to previously fields that were invalid after clicking submit. This change would also be inconsistent with other forms in Kibana. I think it's important to have a consistent approach here.

Good question, @thomheymann.

TL;DR version: It's distracting and annoying to have on-typing/blurring inline validation happening during initial form entry.

Long version: Users tend to work in two different modes when interacting with forms: form entry mode and error fixing mode. Mixing the two simultaneously via immediate inline validation has been found to increase cognitive load on users and actually have the opposite of the desired effect by increasing user errors during the form entry process. Plus, it's annoying for users to be beaten over the head with an error when they're already attempting to edit the field being flagged. I find that delaying the inline validation until after the first submission ends up being the best of all worlds (uninterrupted entry, followed by immediate feedback to correct errors). In this case particular case, assuming it's possible for us to achieve, I'd prefer to apply a better solution for our users than to adopt a poorer one for the sake of consistency.

thomheymann · 2022-05-24T08:04:34Z

@MichaelMarcialis What's the reasoning for changing the logic here to show inline validation errors only after the user clicked submit? The purpose of inline validation is to show errors in context after the user changes a field so that they get immediate feedback and don't have to go back to previously fields that were invalid after clicking submit. This change would also be inconsistent with other forms in Kibana. I think it's important to have a consistent approach here.

Good question, @thomheymann.

TL;DR version: It's distracting and annoying to have on-typing/blurring inline validation happening during initial form entry.

Long version: Users tend to work in two different modes when interacting with forms: form entry mode and error fixing mode. Mixing the two simultaneously via immediate inline validation has been found to increase cognitive load on users and actually have the opposite of the desired effect by increasing user errors during the form entry process. Plus, it's annoying for users to be beaten over the head with an error when they're already attempting to edit the field being flagged. I find that delaying the inline validation until after the first submission ends up being the best of all worlds (uninterrupted entry, followed by immediate feedback to correct errors). In this case particular case, assuming it's possible for us to achieve, I'd prefer to apply a better solution for our users than to adopt a poorer one for the sake of consistency.

Interesting, thanks for explanation @MichaelMarcialis. I think it's worth standardising this behaviour since we now have different form validation behaviours across Kibana (even on the same page when opening the change password modal) which isn't ideal. Is this something the rest of the design team are implementing across the rest of Kibana or is this specific to this page?

thomheymann · 2022-05-24T08:18:07Z

Merging into feature branch to address CI issues there (#132522)

MichaelMarcialis · 2022-05-24T17:05:52Z

Is this something the rest of the design team are implementing across the rest of Kibana or is this specific to this page?

@thomheymann: I'm not currently aware of plans/discussions to change how we approach validation globally across Kibana, but it would be a good one to start. I'll plan to bring the topic up in our next platform design sync. Thanks!

MichaelMarcialis · 2022-05-24T18:29:33Z

@azasypkin, @thomheymann: It looks like my final review comments weren't addressed in this PR, including:

Is the plan to capture them in the feature branch this was merged into? Or are they to be tackled in a separate PR? Just let me know if any issues need to be made. Thanks.

azasypkin · 2022-05-25T12:56:34Z

@azasypkin, @thomheymann: It looks like my final review comments weren't addressed in this PR, including:

Sorry for the confusion and thanks for double checking, @MichaelMarcialis! Yeah, we will handle the remaining feedback in the scope of the separate issue #132645.

thomheymann · 2022-05-26T11:47:12Z

Is this something the rest of the design team are implementing across the rest of Kibana or is this specific to this page?

@thomheymann: I'm not currently aware of plans/discussions to change how we approach validation globally across Kibana, but it would be a good one to start. I'll plan to bring the topic up in our next platform design sync. Thanks!

@MichaelMarcialis That's fair but I'd prefer that discussion to happen before we change the validation logic here and make it inconsistent with the rest of Kibana. If validating forms only after they have been submitted is the direction of travel I agree that it makes sense to iteratively update our forms but it doesn't sounds like that conversation has happened yet.

MichaelMarcialis · 2022-05-26T14:07:55Z

Is this something the rest of the design team are implementing across the rest of Kibana or is this specific to this page?

@thomheymann: I'm not currently aware of plans/discussions to change how we approach validation globally across Kibana, but it would be a good one to start. I'll plan to bring the topic up in our next platform design sync. Thanks!

@MichaelMarcialis That's fair but I'd prefer that discussion to happen before we change the validation logic here and make it inconsistent with the rest of Kibana. If validating forms only after they have been submitted is the direction of travel I agree that it makes sense to iteratively update our forms but it doesn't sounds like that conversation has happened yet.

@thomheymann: I've already added this topic to the platform design team's agenda for discussion in our upcoming weekly sync. As for this particular PR, it appears that @azasypkin has already kindly implemented the suggested updates to the validation methodology. Seeing as it is an improvement to the user experience and the work is already done, I'd prefer we keep those updates intact in this PR and not revert to a less user friendly experience for the sake of consistency. In this particular case, I don't think consistency outweighs a better experience.

thomheymann · 2022-05-26T19:01:14Z

@thomheymann: I'm not currently aware of plans/discussions to change how we approach validation globally across Kibana, but it would be a good one to start. I'll plan to bring the topic up in our next platform design sync. Thanks!

@MichaelMarcialis That's fair but I'd prefer that discussion to happen before we change the validation logic here and make it inconsistent with the rest of Kibana. If validating forms only after they have been submitted is the direction of travel I agree that it makes sense to iteratively update our forms but it doesn't sounds like that conversation has happened yet.

@thomheymann: I've already added this topic to the platform design team's agenda for discussion in our upcoming weekly sync. As for this particular PR, it appears that @azasypkin has already kindly implemented the suggested updates to the validation methodology. Seeing as it is an improvement to the user experience and the work is already done, I'd prefer we keep those updates intact in this PR and not revert to a less user friendly experience for the sake of consistency. In this particular case, I don't think consistency outweighs a better experience.

@MichaelMarcialis It's great that you're improving the user experience with the latest research/data. It's super interesting and I'm sure you are correct with your assumption but there's different views on this since both, inline validation and submit validation have advantages and disadvantages so I would really prefer to have consensus amongst the UX team on how form validation should be handled. Fundamental behaviours like this should be consistent across the app and the scope of this PR is not to run an experiment but to add customisable avatars to the user profile page so I'd prefer to stick to that. Let me know what the outcome of your discussion is.

thomheymann added 3 commits March 14, 2022 16:48

Add user profile UI

2e3a7b1

add code doc

98eb7c7

More

25eaf0b

thomheymann commented Mar 14, 2022

View reviewed changes

thomheymann requested review from azasypkin and jportner March 14, 2022 22:37

thomheymann changed the title ~~Redesign user profile screen to be ES User Profile centric~~ Redesign user profile page Mar 14, 2022

thomheymann changed the title ~~Redesign user profile page~~ Redesigned user profile page Mar 14, 2022

elastic deleted a comment from kibana-ci Mar 15, 2022

thomheymann added 3 commits March 15, 2022 11:30

Fix nav control for anonymous users

87ae3f1

Move presentational logic outside form row

e696ce0

Fix disabled state and update profile api

10f8f9b

azasypkin reviewed Mar 16, 2022

View reviewed changes

thomheymann added 11 commits March 16, 2022 12:51

remove presentational logic from form row

51825b9

wip

6147fb5

Fix initials

f46f750

.

4164504

empty user avatar

d96f416

simplify reserved user screen

1a42bf4

.

b50a171

unit tests

97fe039

type fixes

ea08f16

.

d416345

.

55eab66

jportner reviewed Mar 21, 2022

View reviewed changes

thomheymann added 4 commits March 23, 2022 11:10

.

488c489

.

4e7e819

.

31a1c35

.

5ce63e6

azasypkin mentioned this pull request May 20, 2022

Prevent cloud users from updating their user profile #132622

Merged

jportner added Feature:Security/User Profile and removed Feature:Security/UserSettings labels May 20, 2022

jportner mentioned this pull request May 20, 2022

Support for viewing another user's avatar #132636

Closed

jportner reviewed May 20, 2022

View reviewed changes

jportner approved these changes May 20, 2022

View reviewed changes

legrego mentioned this pull request May 20, 2022

Address review feedback for redesigned user profile page #132645

Closed

azasypkin added 2 commits May 23, 2022 09:25

Merge branch 'feature/user-profile' into pr-127624-user-profiles

73fce79

Review#4: improve warning message for the change password form when c…

0349691

…hanging password for Kibana system users.

MichaelMarcialis reviewed May 23, 2022

View reviewed changes

MichaelMarcialis approved these changes May 23, 2022

View reviewed changes

thomheymann merged commit 1775421 into elastic:feature/user-profile May 24, 2022

MichaelMarcialis mentioned this pull request May 24, 2022

Introduce user profiles #132522

Merged

This was referenced Jun 22, 2022

Changing left and right padding on User Profiles screen #134944

Merged

Removing Flex styled elements #135317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesigned user profile page #127624

Redesigned user profile page #127624

thomheymann commented Mar 14, 2022 •

edited by azasypkin

Loading

thomheymann left a comment

azasypkin commented Mar 16, 2022

azasypkin left a comment

jportner left a comment •

edited

Loading

jportner Mar 21, 2022

thomheymann commented May 19, 2022 •

edited

Loading

azasypkin commented May 20, 2022

jportner left a comment

jportner left a comment •

edited by legrego

Loading

gchaps commented May 20, 2022 •

edited

Loading

jportner commented May 20, 2022

legrego commented May 22, 2022

kibana-ci commented May 23, 2022

MichaelMarcialis May 23, 2022

MichaelMarcialis left a comment •

edited

Loading

MichaelMarcialis commented May 23, 2022 •

edited

Loading

thomheymann commented May 24, 2022

thomheymann commented May 24, 2022

MichaelMarcialis commented May 24, 2022

MichaelMarcialis commented May 24, 2022 •

edited

Loading

azasypkin commented May 25, 2022

thomheymann commented May 26, 2022

MichaelMarcialis commented May 26, 2022

thomheymann commented May 26, 2022

Redesigned user profile page #127624

Redesigned user profile page #127624

Conversation

thomheymann commented Mar 14, 2022 • edited by azasypkin Loading

Profile page (ordinary user)

Profile page (ordinary user, profile picture)

Profile page (ordinary user, change password modal)

Profile page (ordinary user with minimal privileges)

Profile page (reserved user)

Profile page (reserved user authenticated via authenticating proxy)

Profile page (ordinary user authenticated via authenticating proxy)

Form changes

Error state

Reserved user

Get profile API

Testing

thomheymann left a comment

Choose a reason for hiding this comment

azasypkin commented Mar 16, 2022

azasypkin left a comment

Choose a reason for hiding this comment

jportner left a comment • edited Loading

Choose a reason for hiding this comment

jportner Mar 21, 2022

Choose a reason for hiding this comment

thomheymann commented May 19, 2022 • edited Loading

azasypkin commented May 20, 2022

jportner left a comment

Choose a reason for hiding this comment

jportner left a comment • edited by legrego Loading

Choose a reason for hiding this comment

gchaps commented May 20, 2022 • edited Loading

jportner commented May 20, 2022

legrego commented May 22, 2022

kibana-ci commented May 23, 2022

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

History

MichaelMarcialis May 23, 2022

Choose a reason for hiding this comment

MichaelMarcialis left a comment • edited Loading

Choose a reason for hiding this comment

MichaelMarcialis commented May 23, 2022 • edited Loading

thomheymann commented May 24, 2022

thomheymann commented May 24, 2022

MichaelMarcialis commented May 24, 2022

MichaelMarcialis commented May 24, 2022 • edited Loading

azasypkin commented May 25, 2022

thomheymann commented May 26, 2022

MichaelMarcialis commented May 26, 2022

thomheymann commented May 26, 2022

thomheymann commented Mar 14, 2022 •

edited by azasypkin

Loading

jportner left a comment •

edited

Loading

thomheymann commented May 19, 2022 •

edited

Loading

jportner left a comment •

edited by legrego

Loading

gchaps commented May 20, 2022 •

edited

Loading

MichaelMarcialis left a comment •

edited

Loading

MichaelMarcialis commented May 23, 2022 •

edited

Loading

MichaelMarcialis commented May 24, 2022 •

edited

Loading