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

Add field user.roles #917

Merged
merged 2 commits into from
Aug 12, 2020
Merged

Add field user.roles #917

merged 2 commits into from
Aug 12, 2020

Conversation

webmat
Copy link
Contributor

@webmat webmat commented Aug 11, 2020

This array field is meant to capture a list of role names that apply to the user, in an audit log or other RBAC event.

Closes #915

@webmat webmat self-assigned this Aug 11, 2020
@legrego legrego mentioned this pull request Aug 11, 2020
4 tasks
@webmat webmat mentioned this pull request Aug 11, 2020
schemas/user.yml Outdated
Comment on lines 91 to 99
description: >
Array of user roles or groups, at the time of the event.

`user.group.*` fields are meant to capture one group's full details,
rather than capturing an array of groups associated with a user.

When it's necessary to capture a list of roles or groups assigned to the
user at the time an event or audit log is recorded,
use the array field `user.roles`.
Copy link
Contributor Author

@webmat webmat Aug 11, 2020

Choose a reason for hiding this comment

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

@legrego @ebeahan Do you think I'm conflating two things here, by discussing roles and groups together?

Copy link
Member

Choose a reason for hiding this comment

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

I tend to think of groups as a collection of users, whereas a role is a collection of privileges. Groups are often used to model user roles within an organization though.

In my opinion, I think it makes sense to treat these as separate concepts, and only mention roles here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conversely, do you think group.roles would be appropriate, then?

I realize ES doesn't have the concept of groups. But in systems that do, roles/privileges can be granted to a group as well.

Note that I'm just trying to make sure we're not missing anything obvious here. But it's probably fine to move forward with adding only this one field, and only add group.role later, if it becomes necessary.

Copy link
Member

Choose a reason for hiding this comment

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

I thought about if a group.roles field would be appropriate too.

While roles assigned to groups, it's ultimately a user who authenticates and is authorized with the roles group membership grants. So I'm not sure if group.role would be as useful right away, but I also agree we could easily add it later if necessary.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @ebeahan - we can introduce group.role at a later date if the need arises

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've completely removed all talk about groups. Actually the field description is only the short description now:

Array of user roles at the time of the event.

Do you think anything else is needed here?

Copy link
Member

Choose a reason for hiding this comment

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

Concise but clear - I think the short description works.

@webmat
Copy link
Contributor Author

webmat commented Aug 11, 2020

I like that this addition is a laser focused addition of one field. However I'd be curious to know if there's more fields we may want to capture, that are also important to audit logs mentioning user roles.

Copy link
Member

@ebeahan ebeahan 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

@legrego legrego left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the quick turnaround!

@webmat webmat merged commit a0f4e43 into elastic:master Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a user.roles field
3 participants