Skip to content

Conversation

@legrego
Copy link
Member

@legrego legrego commented Oct 23, 2020

Summary

Updates the user and role grid pages to create properly encoded links (Resolves #66412)

@legrego legrego added 1 release_note:fix Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// v7.10.0 v7.11.0 v8.0.0 and removed 1 labels Oct 23, 2020
@legrego legrego changed the title Properly encode links to edit user page Properly encode links to edit user page, fix role display Oct 23, 2020
@legrego legrego changed the title Properly encode links to edit user page, fix role display Properly encode links to edit user page Oct 26, 2020
@legrego legrego marked this pull request as ready for review October 26, 2020 13:49
@legrego legrego requested a review from a team as a code owner October 26, 2020 13:49
@elasticmachine
Copy link
Contributor

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

@azasypkin
Copy link
Member

ACK: reviewing...

@azasypkin azasypkin self-requested a review October 26, 2020 14:40
<EuiLink
data-test-subj="userRowUserName"
{...reactRouterNavigate(this.props.history, `/edit/${username}`)}
{...reactRouterNavigate(this.props.history, `/edit/${encodeURIComponent(username)}`)}
Copy link
Member

Choose a reason for hiding this comment

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

question: how do you feel about doing the same for the role name in Roles Grid? I see we don't do encoding there. I know Kibana UI doesn't allow creating roles with such names, but ES API does. At least I managed to create role with such a name using Kibana DevTools and it broke our UI:

POST /_security/role/my_admin%25role
{
  "cluster": ["all"],
  "indices": [
    {
      "names": [ "index1", "index2" ],
      "privileges": ["all"]
    }
  ],
  "applications": [
    {
      "application": "myapp",
      "privileges": [ "admin", "read" ],
      "resources": [ "*" ]
    }
  ]
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Great idea, will do!

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
security 1.0MB 1.0MB +40.0B

History

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

@legrego legrego merged commit 9f7ccc6 into elastic:master Oct 26, 2020
@legrego legrego deleted the security/username-encoding branch October 26, 2020 18:14
legrego added a commit to legrego/kibana that referenced this pull request Oct 26, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 27, 2020
* master: (37 commits)
  [ILM] Migrate Warm phase to Form Lib (elastic#81323)
  [Security Solutions][Detection Engine] Fixes critical bug with error reporting that was doing a throw (elastic#81549)
  [Detection Rules] Add 7.10 rules (elastic#81676)
  [kbn/optimizer] ignore missing metrics when updating limits with --focus (elastic#81696)
  [SECURITY SOLUTIONS] Bugs overview page + investigate eql in timeline (elastic#81550)
  [Maps] fix unable to edit cluster vector styles styled by count when switching to super fine grid resolution (elastic#81525)
  Fixed migration issue for case specific actions, by extending email action migrator checks (elastic#81673)
  [CI] Preparation for APM tracking on CI (elastic#80399)
  [Home] Fixes Kibana app description order on home page and updates Canvas copy (elastic#80057)
  Make sure `to` is 'now' and not the same as `from` (elastic#81524)
  Nitpicking the 8.0 Breaking Change issue template (elastic#81678)
  [SECURITY_SOLUTION] Fix text on onboarding screen (elastic#81672)
  [data.search] Skip async search tests in build candidates and production builds (elastic#81547)
  Fix previousStartedAt by not changing when execution fails (elastic#81388)
  [Monitoring] Fix a couple of issues with the cpu usage alert (elastic#80737)
  Telemetry collection xpack to ts project references (elastic#81269)
  Elasticsearch: don't use url authentication for new client (elastic#81564)
  [App Search] Credentials: implement working flyout form (elastic#81541)
  Properly encode links to edit user page (elastic#81562)
  [Alerting UI] Don't wait for health check before showing Create Alert flyout (elastic#80996)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_note:fix reverted Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// v7.10.0 v7.11.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security Users UI - error when user name contains %

4 participants