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

Reactify users roles #20739

Merged
merged 32 commits into from
Jul 19, 2018
Merged

Conversation

bmcconaghy
Copy link
Contributor

@bmcconaghy bmcconaghy commented Jul 12, 2018

This PR converts the users list and edit users screen of security management to use EUI. It is intended to be a straight port with no modifications. One exception to this is that the email field for users has been removed (it is not used anywhere).

image

image

image

image

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cjcenizal
Copy link
Contributor

When I review this I'll check to see if this has an effect on #18444 and/or #18443.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@legrego
Copy link
Member

legrego commented Jul 18, 2018

@bmcconaghy are we sure we are ok removing the email address field? Since this is still part of the Elasticsearch User Management API, it seems strange that we don't support it in the UI as at least an optional field.

@cjcenizal
Copy link
Contributor

cjcenizal commented Jul 18, 2018

FYI I confirmed that this PR fixes #20194, #18050, #18288, and #18444.

@bmcconaghy
Copy link
Contributor Author

@legrego added the email field back in

@legrego
Copy link
Member

legrego commented Jul 19, 2018

Thanks, @bmcconaghy! I'll take a look this afternoon

id="username"
name="username"
ng-model="user.username"
required pattern="[a-zA-Z_][a-zA-Z0-9_@\-\$\.]*"
Copy link
Member

Choose a reason for hiding this comment

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

I think we lost this validation -- I can create a user with a username consisting of just special characters. It looks like ES (or maybe Kibana itself) strips out some of those characters, so the username I typed isn't the username that was actually saved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, added that back in.

@@ -96,6 +96,8 @@ export class EditUser extends Component {
const { username } = this.state.user;
if (username !== null && !username) {
return 'Username is required';
} else if (!username.match(validUsernameRegex)) {
return 'Username contains invalid character(s)';
Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry to be such a pest... can we use the original error message here? I feel like it'd be more helpful to users trying to figure out what they did wrong:

Username must begin with a letter or underscore and contain only letters, underscores, and numbers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lol, you're not being a pest, should have just gotten the original error message in the first place.

@jen-huang
Copy link
Contributor

Just pulled this down again and getting a weird error when I click Create new user:

screen shot 2018-07-19 at 12 00 09 pm

screen shot 2018-07-19 at 11 59 57 am

My instance has only system/reserved users.

@bmcconaghy
Copy link
Contributor Author

@jen-huang that's what I get for making changes w/o testing :( . Just pushed a fix.

Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

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

Test locally and again and changes look good! My remaining concern is making the Full name and Email fields optional, but they were required in the previous version too, so not a blocker as this PR is just retaining parity.

LGTM!

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.

LGTM (pending green CI)! Thanks for those revisions, Bill! Tested again locally, seems to do exactly what we need.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@bmcconaghy bmcconaghy merged commit 35dade2 into elastic:master Jul 19, 2018
@bmcconaghy bmcconaghy deleted the reactify_users_roles branch July 19, 2018 20:40
bmcconaghy added a commit to bmcconaghy/kibana that referenced this pull request Jul 20, 2018
* partial progress on reactifying users

* progress on EUIfication of users screen

* removing Angular stuff

* adding data-test-subj="passwordConfirmationInput"

* removing data-test-subj="userFormEmailInput" refs from tests

* fixing selector for role assignment

* some functional test fixes

* fixing some functional tests

* fixing last functional test

* removing stray console log

* fixing warnings

* attempting to fix flaky test

* trying again to fix flaky test

* PR feedback

* PR feedback

* fixing issue where form tried to submit

* adding sleep to allow user to load

* Design edits

Mainly adding wrapper EUI page elements, but also shifted around form elements.

* Fixed console error and added responsive prop to table

* addressing PR feedback

* A few more PR feedback

- Fixed alignment of table
- Removed the tooltip from the lock icon and placed the description inline.
- Changed delete button to an empty button

* addressing more PR feedback

* adding email field back in

* adding back username validation

* restoring original error message

* fixing dumb null error
bmcconaghy added a commit that referenced this pull request Jul 20, 2018
* partial progress on reactifying users

* progress on EUIfication of users screen

* removing Angular stuff

* adding data-test-subj="passwordConfirmationInput"

* removing data-test-subj="userFormEmailInput" refs from tests

* fixing selector for role assignment

* some functional test fixes

* fixing some functional tests

* fixing last functional test

* removing stray console log

* fixing warnings

* attempting to fix flaky test

* trying again to fix flaky test

* PR feedback

* PR feedback

* fixing issue where form tried to submit

* adding sleep to allow user to load

* Design edits

Mainly adding wrapper EUI page elements, but also shifted around form elements.

* Fixed console error and added responsive prop to table

* addressing PR feedback

* A few more PR feedback

- Fixed alignment of table
- Removed the tooltip from the lock icon and placed the description inline.
- Changed delete button to an empty button

* addressing more PR feedback

* adding email field back in

* adding back username validation

* restoring original error message

* fixing dumb null error
legrego added a commit that referenced this pull request Jul 20, 2018
This PR updates the new Role Management screen to use the public role API introduced as part of #20732.

Additionally, this updates the breadcrumb nav for Spaces and Role management screens to be consistent with the nav introduced in #20739.

There are a couple of visual glitches in this PR which are a result of current defects on master, so they can be safely ignored for the time being.
@bhavyarm bhavyarm mentioned this pull request Aug 22, 2018
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants