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

Require that all app privileges have actions #32272

Merged
merged 2 commits into from
Jul 24, 2018

Conversation

tvernum
Copy link
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 kobelb and jaymode July 23, 2018 04:12
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@tvernum
Copy link
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
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
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 java.util.Collections;
import java.util.List;

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

@TestLogging("org.elasticsearch.xpack.core.security.authz.permission:TRACE")
Copy link
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
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