Skip to content

Require that all app privileges have actions#32272

Merged
tvernum merged 2 commits intoelastic:security-app-privsfrom
tvernum:app-privs/none
Jul 24, 2018
Merged

Require that all app privileges have actions#32272
tvernum merged 2 commits intoelastic:security-app-privsfrom
tvernum:app-privs/none

Conversation

@tvernum
Copy link
Copy Markdown
Contributor

@tvernum tvernum commented Jul 23, 2018

The javadoc and validation for ApplicationPrivileges supports the idea
that a privilege could have no actions. However, in that case every
privilege that had no actions grants every other action-less privilege
within the same action, including the NONE privilege.

This commit makes the following changes:

  • It is not possible to PUT a privilege without any actions
  • A permission with no actions, never grants another privilege even if
    that privilege is also a zero-action privilege (which, ideally would
    never exist, but can occur through missing privileges or index
    manipulation).

The javadoc and validation for ApplicationPrivileges supports the idea
that a privilege could have no actions. However, in that case every
privilege that had no actions grants every other action-less privilege
within the same action, including the NONE privilege.

This commit makes the following changes:
- It is not possible to PUT a privilege without any actions
- A permission with no actions, never grants another privilege even if
  that privilege is also a zero-action privilege (which, ideally would
  never exist, but can occur through missing privileges or index
  manipulation).
@tvernum tvernum added review :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC labels Jul 23, 2018
@tvernum tvernum requested review from jaymode and kobelb July 23, 2018 04:12
@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-security

@tvernum
Copy link
Copy Markdown
Contributor Author

tvernum commented Jul 23, 2018

This change was triggered by conversation here:
https://github.com/elastic/elasticsearch/pull/32137/files#r203572756

Copy link
Copy Markdown
Contributor

@kobelb kobelb left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

LGTM

import static java.util.Collections.singletonList;
import static org.hamcrest.Matchers.equalTo;

@TestLogging("org.elasticsearch.xpack.core.security.authz.permission:TRACE")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do we still need this test logging?

@tvernum
Copy link
Copy Markdown
Contributor Author

tvernum commented Jul 24, 2018

@elasticmachine run gradle build tests
(failed because has-privileges doesn't support GET with source param).

@tvernum tvernum merged commit 6087767 into elastic:security-app-privs Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants