feat: allow account role to be set to none #431
Conversation
|
does this need to be exposed via also: reminder to test the locally built |
|
|
||
| func getAccountRole(ctx context.Context, client authservice.AuthServiceClient, actionGroup string) (*auth.Role, error) { | ||
| func getAccountRole(ctx context.Context, client authservice.AuthServiceClient, actionGroup string, allowNone bool) (*auth.Role, error) { | ||
| if allowNone && strings.ToLower(strings.TrimSpace(actionGroup)) == accountActionGroupNone { |
There was a problem hiding this comment.
suggestion: i'd rename the allowNone -> allowUnspecified and remain consistent to with the enum definition.
There was a problem hiding this comment.
Unspecified is different than none. In this case the user should have no actual account role, it's not that they have an account role of unspecified. none is just the parameter used to indicate that no roles should be specified.
| return err | ||
| } | ||
| if accountRoleToSet.Spec.AccountRole.ActionGroup == auth.ACCOUNT_ACTION_GROUP_ADMIN { | ||
| if accountRoleToSet == nil { |
There was a problem hiding this comment.
if possible, i'd recommend having getAccountRole return auth.ACCOUNT_ACTION_GROUP_UNSPECIFIED instead of nil.
also: do we need this additional if branch? seems like we can use the existing else branch and skip appending the accountRoleToSet if the account action group is UNSPECIFIED.
There was a problem hiding this comment.
Similarly, UNSPECIFIED is a valid value in the enum. none exists because it shouldn't have any roles.
This is specifically to allow users who existed before SCIM and have an account role to be removed from their roles so that they are like new SCIM users. |
|
|
||
| // first get the required account role | ||
| role, err := getAccountRole(c.ctx, c.client, accountRole) | ||
| role, err := getAccountRole(c.ctx, c.client, accountRole, false) |
There was a problem hiding this comment.
would this throw a nil pointer exception below?
roleIDs = append(roleIDs, role.GetId())
There was a problem hiding this comment.
This doesn't allow nil to be returned because none is invalid for inviting a user. It'd get an error.
| } | ||
|
|
||
| // the role ids to invite the users for | ||
| var roleIDs []string |
There was a problem hiding this comment.
just so i understand--we are allowing a state where we can set account role to nil--and namespace roles to not nil?
There was a problem hiding this comment.
Also do we need to update the access package validation to allow the account role to be empty?
There was a problem hiding this comment.
Yes, account role can be nil, and namespace roles can be empty or not empty.
The access validation already supports no account role if the user is a SCIM user.
What was changed
If a user was added via SCIM, setting their account role to none is valid. The call will fail if the user is not a SCIM user.
Why?
So user's can have only group derived roles assignments.
Checklist
Unit test and manual test of SCIM user