Skip to content

feat: allow account role to be set to none #431

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.

Sign up for GitHub

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

Open
captainbeardo wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: allow account role to be set to none #431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b254b9
feat: allow account role to be set to none
captainbeardo Mar 5, 2025
6ce7927
Merge branch 'main' into beardo/user-account-role-none
captainbeardo Apr 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion app/role.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ var (
namespaceActionGroups = getNamespaceActionGroups()
)

const (
accountActionGroupNone = "none"
)

func getAccountActionGroups() []string {
var rv []string
for n, v := range auth.AccountActionGroup_value {
Expand Down Expand Up @@ -63,7 +67,10 @@ func getNamespaceRolesBatch(ctx context.Context, client authservice.AuthServiceC
return res.Roles, nil
}

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 {
Copy link
Contributor

@shakeelrao shakeelrao Mar 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

suggestion: i'd rename the allowNone -> allowUnspecified and remain consistent to with the enum definition.

Copy link
Contributor Author

@captainbeardo captainbeardo Apr 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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 nil, nil
}
ag, err := toAccountActionGroup(actionGroup)
if err != nil {
return nil, err
Expand Down
16 changes: 13 additions & 3 deletions app/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ func (c *UserClient) inviteUsers(
var roleIDs []string
Copy link
Member

@mattkim mattkim Apr 30, 2025

Choose a reason for hiding this comment

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

just so i understand--we are allowing a state where we can set account role to nil--and namespace roles to not nil?

Copy link
Member

@mattkim mattkim Apr 30, 2025

Choose a reason for hiding this comment

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

Also do we need to update the access package validation to allow the account role to be empty?

Copy link
Contributor Author

@captainbeardo captainbeardo Apr 30, 2025

Choose a reason for hiding this comment

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

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.


// first get the required account role
role, err := getAccountRole(c.ctx, c.client, accountRole)
role, err := getAccountRole(c.ctx, c.client, accountRole, false)
Copy link
Member

@mattkim mattkim Apr 30, 2025

Choose a reason for hiding this comment

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

would this throw a nil pointer exception below?

roleIDs = append(roleIDs, role.GetId())

Copy link
Contributor Author

@captainbeardo captainbeardo Apr 30, 2025

Choose a reason for hiding this comment

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

This doesn't allow nil to be returned because none is invalid for inviting a user. It'd get an error.

if err != nil {
return err
}
Expand Down Expand Up @@ -257,11 +257,21 @@ func (c *UserClient) setAccountRole(
return err
}
var newRoleIDs []string
accountRoleToSet, err := getAccountRole(c.ctx, c.client, accountRole)
accountRoleToSet, err := getAccountRole(c.ctx, c.client, accountRole, true)
if err != nil {
return err
}
if accountRoleToSet.Spec.AccountRole.ActionGroup == auth.ACCOUNT_ACTION_GROUP_ADMIN {
if accountRoleToSet == nil {
Copy link
Contributor

@shakeelrao shakeelrao Mar 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@captainbeardo captainbeardo Apr 30, 2025

Choose a reason for hiding this comment

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

Similarly, UNSPECIFIED is a valid value in the enum. none exists because it shouldn't have any roles.

// Setting account role to none.
for _, r := range userRoles {
// remove any account roles
if r.Type == auth.ROLE_TYPE_PREDEFINED && r.Spec.AccountRole != nil {
continue
} else {
newRoleIDs = append(newRoleIDs, r.Id)
}
}
} else if accountRoleToSet.Spec.AccountRole.ActionGroup == auth.ACCOUNT_ACTION_GROUP_ADMIN {
// set the user account admin role
y, err := ConfirmPrompt(ctx, "Setting admin role on user. All existing namespace permissions will be replaced, please confirm")
if err != nil {
Expand Down
55 changes: 53 additions & 2 deletions app/user_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,16 @@ package app
import (
"context"
"errors"
"reflect"
"testing"

"github.com/golang/mock/gomock"
"github.com/stretchr/testify/suite"
"github.com/temporalio/tcld/protogen/api/auth/v1"
"github.com/temporalio/tcld/protogen/api/authservice/v1"
"github.com/temporalio/tcld/protogen/api/request/v1"
authservicemock "github.com/temporalio/tcld/protogen/apimock/authservice/v1"
"github.com/urfave/cli/v2"
"reflect"
"testing"
)

func TestUser(t *testing.T) {
Expand Down Expand Up @@ -397,6 +398,56 @@ func (s *UserTestSuite) TestSetAccountRoleAdmin() {
s.NoError(s.RunCmd("user", "set-account-role", "--user-email", "test@example.com", "--account-role", "Admin"))
}

func (s *UserTestSuite) TestSetAccountRoleNone() {
s.mockAuthService.EXPECT().GetUser(gomock.Any(), gomock.Any()).Return(&authservice.GetUserResponse{
User: &auth.User{
Id: "test-user-id",
Spec: &auth.UserSpec{
Email: "test@example.com",
},
},
}, nil).Times(1)
s.mockAuthService.EXPECT().GetRoles(gomock.Any(), gomock.Any()).Return(&authservice.GetRolesResponse{
Roles: []*auth.Role{
{
Id: "test-account-developer-role",
Type: auth.ROLE_TYPE_PREDEFINED,
Spec: &auth.RoleSpec{
AccountRole: &auth.AccountRoleSpec{
ActionGroup: auth.ACCOUNT_ACTION_GROUP_DEVELOPER,
},
},
},
{
Id: "test-namespace-admin-role",
Type: auth.ROLE_TYPE_PREDEFINED,
Spec: &auth.RoleSpec{
NamespaceRoles: []*auth.NamespaceRoleSpec{
{
Namespace: "ns1",
ActionGroup: auth.NAMESPACE_ACTION_GROUP_ADMIN,
},
},
},
},
},
}, nil).Times(1)
s.mockAuthService.EXPECT().UpdateUser(gomock.Any(), gomock.All(&updateUserRequestMatcher{
request: &authservice.UpdateUserRequest{
UserId: "test-user-id",
Spec: &auth.UserSpec{
Email: "test@example.com",
Roles: []string{"test-namespace-admin-role"},
},
},
})).Return(&authservice.UpdateUserResponse{
RequestStatus: &request.RequestStatus{
State: request.STATE_FULFILLED,
},
}, nil)
s.NoError(s.RunCmd("user", "set-account-role", "--user-email", "test@example.com", "--account-role", "none"))
}

func (s *UserTestSuite) TestSetNamespacePermissions() {
s.mockAuthService.EXPECT().GetUser(gomock.Any(), gomock.Any()).Return(&authservice.GetUserResponse{
User: &auth.User{
Expand Down