-
Notifications
You must be signed in to change notification settings - Fork 162
Custom user attributes support #8120
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
Conversation
fc421dc to
13718d4
Compare
13718d4 to
92340e2
Compare
begelundmuller
left a comment
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.
It also needs CLI support for viewing attributes (probably in rill user list?) and updating attributes (maybe rill user set-attributes?)
Happy to add it in this PR, I was going to just do it in a follow up |
populate the default attributes if they are nil support custom attributes overrides
Update the query to use a wildcard for user email matching.
begelundmuller
left a comment
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.
Can you also add an integration test for this feature in admin/server/rbac_test.go? Unlike the Postgres DB tests, that tests at the API level and covers permission checks, etc.
5dd913a to
da706eb
Compare
admin/server/organizations.go
Outdated
|
|
||
| // Find the organization member | ||
| members, err := s.admin.DB.FindOrganizationMemberUsers(ctx, org.ID, "", true, user.Email, 1, "") | ||
| members, err := s.admin.DB.FindOrganizationMemberUsers(ctx, org.ID, "", true, "", 1, user.Email) |
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.
It's not safe to rely on a search pattern for a single-row lookup. For example, emails can have a % in them. Maybe add a proper singular FindOrganizationMemberUser DB query?
admin/server/rbac_test.go
Outdated
| require.Equal(t, "123", resp.Member.Attributes.Fields["restaurant_id"].GetStringValue()) | ||
| require.Equal(t, "engineering", resp.Member.Attributes.Fields["department"].GetStringValue()) |
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.
Convert the proto types to Go types instead of using them directly. In this case, could it just do require.Equal(t, attrs, resp.Member.Attributes.AsMap())?
admin/server/rbac_test.go
Outdated
| require.Equal(t, "456", updatedResp.Member.Attributes.Fields["restaurant_id"].GetStringValue()) | ||
| require.Equal(t, "platform", updatedResp.Member.Attributes.Fields["team"].GetStringValue()) | ||
| require.False(t, updatedResp.Member.Attributes.Fields["department"] != nil) |
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.
Same question
e5160e9 to
3f497bb
Compare
Adds backend support for adding custom attributes that propagate in a JWT
as_sql_listIn another PR I'd like to add CLI commands for attribute management and bulk import / export
Checklist: