-
Notifications
You must be signed in to change notification settings - Fork 419
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
Add field user.roles
#917
Conversation
schemas/user.yml
Outdated
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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
There was a problem hiding this 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!
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