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

Warn & Disallow Creating Role with Existing Name #132218

Merged
merged 7 commits into from
May 25, 2022

Conversation

jeramysoucy
Copy link
Contributor

@jeramysoucy jeramysoucy commented May 16, 2022

Resolves #34712

Overview

This PR adds checks to the edit role page, when in create mode, to warn the user and disallow role creation when the user enters a role name that already exists. This is achieved in two ways...

When the name field loses focus:
A new state property and helper function are utilized to flag when the entered name already exists. If the page is in create mode, the state property is checked and set in the onBlur handler for the name field. When the form row containing the field is rendered the isValid and error are set based on the new state property, providing in-line feedback in the form.

When the create button is clicked:
An API get call already occurs during the save operation and is leveraged in this change. A new optional 'createOnly' parameter is passed into the client API saveRole method. The value for createOnly is determined by the page being in create mode (!isEditingExistingRole). The onSave method will throw a conflict (409) error if the create only parameter is true and the role name already exists. The thrown error is displayed in a toast notification.

Non-breaking API Change

The API saveRole method and PUT /api/security/role/ request are both modified with an an optional boolean parameter 'createOnly'. 'createOnly' has a default value of false to avoid a breaking change with any existing references to the method. If 'createOnly' is true, a response/result of 409 will indicate the role cannot be created because it already exists.

Testing

  1. Create a role and provide some known name (e.g. 'test_role')
  2. In the Create Role UI, enter the same name and press tab or click outside of the name field
  3. Verify that an inline message appears warning that the name already exists
  4. Change one character in the name field such that it does not match any existing role names (note: the field is case-sensitive)
  5. Verify the inline message is no longer preset
  6. Enter the name of the role created in step 1, scroll to the Create button and click it
  7. Verify a toast notification appears warning that the name already exists
  8. Change one character in the name field such that it does not match any existing role names. Click the Create button.
  9. Verify the new role is created
  10. Select an existing role to modify it
  11. Verify the field can be modified normally without receiving any feedback that the name already exists

@jeramysoucy jeramysoucy added release_note:enhancement Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.3.0 Feature:Users/Roles/API Keys labels May 16, 2022
…when attempting to create a role with a name that already exists.

Disallows creating a role with a name that already exists.
Event handling efficiency needs review.
Implemented unit and functional tests.
@jeramysoucy jeramysoucy marked this pull request as ready for review May 19, 2022 13:29
@jeramysoucy jeramysoucy requested a review from a team as a code owner May 19, 2022 13:29
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

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

Nice work, a few nits and suggestions in the comments below 👍

@jeramysoucy
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

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

A couple more nits, LGTM, nice job!!

jeramysoucy and others added 2 commits May 25, 2022 16:33
…test.ts

Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>
…_role_page.test.tsx

Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>
@jeramysoucy jeramysoucy enabled auto-merge (squash) May 25, 2022 14:37
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
security 472.0KB 472.5KB +445.0B

Page load bundle

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

id before after diff
security 50.2KB 50.2KB +37.0B

History

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

@jeramysoucy jeramysoucy merged commit cb403a6 into elastic:main May 25, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 25, 2022
@jportner jportner added v8.4.0 auto-backport Deprecated - use backport:version if exact versions are needed and removed backport:skip This commit does not require backporting labels Jun 6, 2022
@jportner
Copy link
Contributor

jportner commented Jun 6, 2022

This PR was mis-labeled 🙈 it actually merged into main after the 8.3 branch had been cut. It should be backported to 8.3.

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.4 The branch "8.4" is invalid or doesn't exist

Manual backport

To create the backport manually run:

node scripts/backport --pr 132218

Questions ?

Please refer to the Backport tool documentation

@jeramysoucy jeramysoucy deleted the warn-role-exists branch June 6, 2022 14:12
jeramysoucy added a commit to jeramysoucy/kibana that referenced this pull request Jun 6, 2022
* Adds inline warning (name focus/onBlur) and toast warning (saveRole) when attempting to create a role with a name that already exists.
Disallows creating a role with a name that already exists.
Event handling efficiency needs review.

* Updated API documentation.
Implemented unit and functional tests.

* Added name compare to throttle GET request from onBlur for efficiency.

* Reorganized functional and unit tests. Improved UI logic and presentation.

* Update x-pack/plugins/security/server/routes/authorization/roles/put.test.ts

Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>

* Update x-pack/plugins/security/public/management/roles/edit_role/edit_role_page.test.tsx

Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>
(cherry picked from commit cb403a6)
jeramysoucy added a commit that referenced this pull request Jun 6, 2022
* Adds inline warning (name focus/onBlur) and toast warning (saveRole) when attempting to create a role with a name that already exists.
Disallows creating a role with a name that already exists.
Event handling efficiency needs review.

* Updated API documentation.
Implemented unit and functional tests.

* Added name compare to throttle GET request from onBlur for efficiency.

* Reorganized functional and unit tests. Improved UI logic and presentation.

* Update x-pack/plugins/security/server/routes/authorization/roles/put.test.ts

Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>

* Update x-pack/plugins/security/public/management/roles/edit_role/edit_role_page.test.tsx

Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>
(cherry picked from commit cb403a6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Users/Roles/API Keys release_note:enhancement Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.3.0 v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Role Management - Warn when role already exists
5 participants