Validate field permissions when creating a role #48108
Conversation
When creating a role, we do not check if the exceptions for the field permissions is a subset of granted fields. If such role is assigned to a user the user authentication fails. On the same lines we validate role query, this commit adds check if the exceptions for the field permissions is a subset of granted fields when parsing the index privileges.
bizybot
added
>bug
:Security/Authorization
|
Pinging @elastic/es-security (:Security/Authorization) |
|
@elasticmachine run elasticsearch-ci/2 elasticsearch-ci/packaging-sample-matrix |
|
@elasticmachine run elasticsearch-ci/packaging-sample-matrix |
|
Is this the behaviour we want? It seems wrong to me. If I have a role with: then the meaning is clear - the role can see fields that start with but why do we want to enforce that? Why isn't the solution to fix FLS to allow this? |
|
@tvernum Thank you for your comment.
Right now this is what the behavior is, we enforce this in the I hope this is okay. |
And my question is, is pushing the validation up the right fix, or is it better to change the validation to accept this? |
|
To be clear, my question is a genuine one. I'm curious as to whether, on balance people think this validation is good because leniency is dangerous and should be avoided, or bad because it's placing the burden on users to solve something that computers can do. I can see an argument for "it should be strict and leave no room for ambiguity" or for "it should accept any values that make sense", I'd like us to take a moment to weigh up those arguments. |
|
I spoke to @bizybot offline. I'm happy to proceed with this in its current form. |
| throw new ElasticsearchParseException("failed to parse indices privileges for role [{}]. [{}] field values [{}] must be a " + | ||
| "subset of [{}] field values [{}]", roleName, Fields.EXCEPT_FIELDS, Strings.arrayToCommaDelimitedString(deniedFields), | ||
| Fields.GRANT_FIELDS, Strings.arrayToCommaDelimitedString(grantedFields)); | ||
| } |
There was a problem hiding this comment.
Can we find a way to unify (some of) this with what's already in FieldPermissions. It doesn't seem right to have to checks for the same requirement implemented separately.
|
Replaced by #50212 |
When creating a role, we do not check if the exceptions for
the field permissions are a subset of granted fields. If such
a role is assigned to a user then that user's authentication fails
for this reason.
We added a check to validate role query in #46275 and on the same lines,
this commit adds check if the exceptions for the field
permissions is a subset of granted fields when parsing the
index privileges from the role descriptor.