Skip to content

refactor(group): remove AddGroupUsers handler + authz entry #1614

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

Merged
whoAbhishekSah merged 3 commits into from
May 15, 2026
Merged

refactor(group): remove AddGroupUsers handler + authz entry #1614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a0f89a
refactor(group): remove AddGroupUsers handler + authz entry
whoAbhishekSah May 14, 2026
663d670
test(e2e): migrate AddGroupUsers callers to SetGroupMemberRole
whoAbhishekSah May 14, 2026
f357bb9
chore: bump PROTON_COMMIT to merged main (795c70f)
whoAbhishekSah May 15, 2026
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
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ TAG := $(shell git rev-list --tags --max-count=1)
VERSION := $(shell git describe --tags ${TAG})
.PHONY: build check fmt lint test test-race vet test-cover-html help install proto admin-app compose-up-dev
.DEFAULT_GOAL := build
PROTON_COMMIT := "859ba765e6cfd44736ddcf42664b742fe7fd916e"
PROTON_COMMIT := "795c70f359264a3c21a4c6412097f139dfc387e6"

admin-app:
@echo " > generating admin build"
Expand Down
49 changes: 0 additions & 49 deletions internal/api/v1beta1connect/group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,55 +392,6 @@ func (h *ConnectHandler) ListGroupUsers(ctx context.Context, request *connect.Re
}), nil
}

func (h *ConnectHandler) AddGroupUsers(ctx context.Context, request *connect.Request[frontierv1beta1.AddGroupUsersRequest]) (*connect.Response[frontierv1beta1.AddGroupUsersResponse], error) {
errorLogger := NewErrorLogger()

_, err := h.orgService.Get(ctx, request.Msg.GetOrgId())
if err != nil {
switch {
case errors.Is(err, organization.ErrDisabled):
return nil, connect.NewError(connect.CodeNotFound, ErrOrgDisabled)
case errors.Is(err, organization.ErrNotExist):
return nil, connect.NewError(connect.CodeNotFound, ErrOrgNotFound)
default:
errorLogger.LogServiceError(ctx, request, "AddGroupUsers.Get", err,
"org_id", request.Msg.GetOrgId())
return nil, connect.NewError(connect.CodeInternal, ErrInternalServerError)
}
}

var joinedErr error
for _, userID := range request.Msg.GetUserIds() {
if err := h.membershipService.SetGroupMemberRole(ctx, request.Msg.GetId(), userID, schema.UserPrincipal, schema.GroupMemberRole); err != nil {
joinedErr = errors.Join(joinedErr, err)
}
}
if joinedErr != nil {
errorLogger.LogServiceError(ctx, request, "AddGroupUsers.SetGroupMemberRole", joinedErr,
"group_id", request.Msg.GetId(),
"user_ids", request.Msg.GetUserIds())

// errors.Is walks the joined error tree; the first matching case wins.
// If multiple users failed with different errors, callers see the most
// specific code we recognize rather than a generic Internal.
switch {
case errors.Is(joinedErr, group.ErrNotExist), errors.Is(joinedErr, group.ErrInvalidID), errors.Is(joinedErr, group.ErrInvalidUUID):
return nil, connect.NewError(connect.CodeNotFound, ErrGroupNotFound)
case errors.Is(joinedErr, user.ErrNotExist):
return nil, connect.NewError(connect.CodeNotFound, ErrUserNotExist)
case errors.Is(joinedErr, user.ErrDisabled):
return nil, connect.NewError(connect.CodeFailedPrecondition, user.ErrDisabled)
case errors.Is(joinedErr, membership.ErrNotOrgMember):
return nil, connect.NewError(connect.CodeFailedPrecondition, ErrNotOrgMember)
case errors.Is(joinedErr, membership.ErrLastGroupOwnerRole):
return nil, connect.NewError(connect.CodeFailedPrecondition, ErrLastGroupOwnerRole)
default:
return nil, connect.NewError(connect.CodeInternal, ErrInternalServerError)
}
}
return connect.NewResponse(&frontierv1beta1.AddGroupUsersResponse{}), nil
}

func (h *ConnectHandler) RemoveGroupUser(ctx context.Context, request *connect.Request[frontierv1beta1.RemoveGroupUserRequest]) (*connect.Response[frontierv1beta1.RemoveGroupUserResponse], error) {
errorLogger := NewErrorLogger()

Expand Down
143 changes: 0 additions & 143 deletions internal/api/v1beta1connect/group_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1236,149 +1236,6 @@ func TestConnectHandler_ListGroupUsers(t *testing.T) {
}
}

func TestConnectHandler_AddGroupUsers(t *testing.T) {
someGroupID := utils.NewString()
someUserID := utils.NewString()
tests := []struct {
name string
setup func(ms *mocks.MembershipService, os *mocks.OrganizationService)
request *connect.Request[frontierv1beta1.AddGroupUsersRequest]
want *connect.Response[frontierv1beta1.AddGroupUsersResponse]
wantErr error
}{
{
name: "should return error if org does not exist",
setup: func(_ *mocks.MembershipService, os *mocks.OrganizationService) {
os.EXPECT().Get(mock.Anything, testOrgID).Return(organization.Organization{}, organization.ErrNotExist)
},
request: connect.NewRequest(&frontierv1beta1.AddGroupUsersRequest{
Id: someGroupID,
OrgId: testOrgID,
}),
want: nil,
wantErr: connect.NewError(connect.CodeNotFound, ErrOrgNotFound),
},
{
name: "should return error if org is disabled",
setup: func(_ *mocks.MembershipService, os *mocks.OrganizationService) {
os.EXPECT().Get(mock.Anything, testOrgID).Return(organization.Organization{}, organization.ErrDisabled)
},
request: connect.NewRequest(&frontierv1beta1.AddGroupUsersRequest{
Id: someGroupID,
OrgId: testOrgID,
}),
want: nil,
wantErr: connect.NewError(connect.CodeNotFound, ErrOrgDisabled),
},
{
name: "should return internal server error if SetGroupMemberRole fails for any user",
setup: func(ms *mocks.MembershipService, os *mocks.OrganizationService) {
os.EXPECT().Get(mock.Anything, testOrgID).Return(testOrgMap[testOrgID], nil)
ms.EXPECT().SetGroupMemberRole(mock.Anything, someGroupID, someUserID, schema.UserPrincipal, schema.GroupMemberRole).Return(errors.New("some error"))
},
request: connect.NewRequest(&frontierv1beta1.AddGroupUsersRequest{
Id: someGroupID,
OrgId: testOrgID,
UserIds: []string{someUserID},
}),
want: nil,
wantErr: connect.NewError(connect.CodeInternal, ErrInternalServerError),
},
{
name: "should return success if SetGroupMemberRole succeeds for each user",
setup: func(ms *mocks.MembershipService, os *mocks.OrganizationService) {
os.EXPECT().Get(mock.Anything, testOrgID).Return(testOrgMap[testOrgID], nil)
ms.EXPECT().SetGroupMemberRole(mock.Anything, someGroupID, someUserID, schema.UserPrincipal, schema.GroupMemberRole).Return(nil)
},
request: connect.NewRequest(&frontierv1beta1.AddGroupUsersRequest{
Id: someGroupID,
OrgId: testOrgID,
UserIds: []string{someUserID},
}),
want: connect.NewResponse(&frontierv1beta1.AddGroupUsersResponse{}),
wantErr: nil,
},
{
name: "should return not found if group does not exist",
setup: func(ms *mocks.MembershipService, os *mocks.OrganizationService) {
os.EXPECT().Get(mock.Anything, testOrgID).Return(testOrgMap[testOrgID], nil)
ms.EXPECT().SetGroupMemberRole(mock.Anything, someGroupID, someUserID, schema.UserPrincipal, schema.GroupMemberRole).Return(group.ErrNotExist)
},
request: connect.NewRequest(&frontierv1beta1.AddGroupUsersRequest{
Id: someGroupID,
OrgId: testOrgID,
UserIds: []string{someUserID},
}),
want: nil,
wantErr: connect.NewError(connect.CodeNotFound, ErrGroupNotFound),
},
{
name: "should return not found if user does not exist",
setup: func(ms *mocks.MembershipService, os *mocks.OrganizationService) {
os.EXPECT().Get(mock.Anything, testOrgID).Return(testOrgMap[testOrgID], nil)
ms.EXPECT().SetGroupMemberRole(mock.Anything, someGroupID, someUserID, schema.UserPrincipal, schema.GroupMemberRole).Return(user.ErrNotExist)
},
request: connect.NewRequest(&frontierv1beta1.AddGroupUsersRequest{
Id: someGroupID,
OrgId: testOrgID,
UserIds: []string{someUserID},
}),
want: nil,
wantErr: connect.NewError(connect.CodeNotFound, ErrUserNotExist),
},
{
name: "should return failed precondition if user is not a member of the org",
setup: func(ms *mocks.MembershipService, os *mocks.OrganizationService) {
os.EXPECT().Get(mock.Anything, testOrgID).Return(testOrgMap[testOrgID], nil)
ms.EXPECT().SetGroupMemberRole(mock.Anything, someGroupID, someUserID, schema.UserPrincipal, schema.GroupMemberRole).Return(membership.ErrNotOrgMember)
},
request: connect.NewRequest(&frontierv1beta1.AddGroupUsersRequest{
Id: someGroupID,
OrgId: testOrgID,
UserIds: []string{someUserID},
}),
want: nil,
wantErr: connect.NewError(connect.CodeFailedPrecondition, ErrNotOrgMember),
},
{
name: "should return failed precondition if demoting last group owner",
setup: func(ms *mocks.MembershipService, os *mocks.OrganizationService) {
os.EXPECT().Get(mock.Anything, testOrgID).Return(testOrgMap[testOrgID], nil)
ms.EXPECT().SetGroupMemberRole(mock.Anything, someGroupID, someUserID, schema.UserPrincipal, schema.GroupMemberRole).Return(membership.ErrLastGroupOwnerRole)
},
request: connect.NewRequest(&frontierv1beta1.AddGroupUsersRequest{
Id: someGroupID,
OrgId: testOrgID,
UserIds: []string{someUserID},
}),
want: nil,
wantErr: connect.NewError(connect.CodeFailedPrecondition, ErrLastGroupOwnerRole),
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
mockMembershipSvc := new(mocks.MembershipService)
mockOrgSvc := new(mocks.OrganizationService)
if tt.setup != nil {
tt.setup(mockMembershipSvc, mockOrgSvc)
}
h := ConnectHandler{
membershipService: mockMembershipSvc,
orgService: mockOrgSvc,
}
got, err := h.AddGroupUsers(context.Background(), tt.request)
if tt.wantErr != nil {
assert.Error(t, err)
assert.Equal(t, tt.wantErr.(*connect.Error).Code(), err.(*connect.Error).Code())
assert.Equal(t, tt.wantErr.(*connect.Error).Message(), err.(*connect.Error).Message())
} else {
assert.NoError(t, err)
assert.EqualValues(t, tt.want, got)
}
})
}
}

func TestConnectHandler_RemoveGroupUser(t *testing.T) {
randomID := utils.NewString()

Expand Down
4 changes: 0 additions & 4 deletions pkg/server/connect_interceptors/authorization.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,10 +466,6 @@ var authorizationValidationMap = map[string]func(ctx context.Context, handler *v
pbreq := req.(*connect.Request[frontierv1beta1.ListGroupUsersRequest])
return handler.IsAuthorized(ctx, relation.Object{Namespace: schema.GroupNamespace, ID: pbreq.Msg.GetId()}, schema.GetPermission, req)
},
"/raystack.frontier.v1beta1.FrontierService/AddGroupUsers": func(ctx context.Context, handler *v1beta1connect.ConnectHandler, req connect.AnyRequest) error {
pbreq := req.(*connect.Request[frontierv1beta1.AddGroupUsersRequest])
return handler.IsAuthorized(ctx, relation.Object{Namespace: schema.GroupNamespace, ID: pbreq.Msg.GetId()}, schema.UpdatePermission, req)
},
"/raystack.frontier.v1beta1.FrontierService/RemoveGroupUser": func(ctx context.Context, handler *v1beta1connect.ConnectHandler, req connect.AnyRequest) error {
pbreq := req.(*connect.Request[frontierv1beta1.RemoveGroupUserRequest])
return handler.IsAuthorized(ctx, relation.Object{Namespace: schema.GroupNamespace, ID: pbreq.Msg.GetId()}, schema.UpdatePermission, req)
Expand Down
Loading
Loading