chore: fix comment

Signed-off-by: Alessandro Yuichi Okimoto <yuichijpn@gmail.com>

Author: cre8ivejp

pkg/account/api/admin_account.go

@@ -418,7 +418,7 @@ func (s *AccountService) getMyOrganizations(
 		if accWithOrg.AccountV2.Disabled || accWithOrg.Organization.Disabled || accWithOrg.Organization.Archived {
 			continue
 		}
-		// If the user is an admin or owner, no need to filter environments.
+		// Add the organization if the account is an admin or owner.
 		// Otherwise, we check if the account is enabled in any environment in this organization.
 		if accWithOrg.AccountV2.OrganizationRole == accountproto.AccountV2_Role_Organization_ADMIN ||
 			accWithOrg.AccountV2.OrganizationRole == accountproto.AccountV2_Role_Organization_OWNER {

pkg/environment/api/organization.go

@@ -875,14 +875,17 @@ func (s *EnvironmentService) UpdateOrganization(
 	req *environmentproto.UpdateOrganizationRequest,
 ) (*environmentproto.UpdateOrganizationResponse, error) {
 	localizer := locale.NewLocalizer(ctx)
-	editor, err := s.checkSystemAdminRole(ctx, localizer)
+	editor, err := s.checkOrganizationRole(ctx, req.Id, accountproto.AccountV2_Role_Organization_OWNER, localizer)
 	if err != nil {
-		// If not system admin, check if user is organization owner
-		editor, err = s.checkOrganizationRole(ctx, req.Id, accountproto.AccountV2_Role_Organization_OWNER, localizer)
-		if err != nil {
+		return nil, err
+	}
+	// Additional security validations for ownership transfer
+	if req.OwnerEmail != nil || (req.ChangeOwnerEmailCommand != nil && req.ChangeOwnerEmailCommand.OwnerEmail != "") {
+		if err := s.validateOwnershipTransfer(ctx, req, editor, localizer); err != nil {
 			return nil, err
 		}
 	}
+
 	commands := s.getUpdateOrganizationCommands(req)
 	if len(commands) == 0 {
 		return s.updateOrganizationNoCommand(ctx, req, editor, localizer)
@@ -942,8 +945,8 @@ func (s *EnvironmentService) updateOrganizationNoCommand(
 			return err
 		}
 		// Set the new owner email if it changes
-		if prevOwnerEmail != organization.OwnerEmail {
-			newOwnerEmail = organization.OwnerEmail
+		if prevOwnerEmail != updated.OwnerEmail {
+			newOwnerEmail = updated.OwnerEmail
 		}
 		return orgStorage.UpdateOrganization(ctxWithTx, updated)
 	})
@@ -1186,31 +1189,109 @@ func (s *EnvironmentService) updateOrganization(
 	return nil
 }
 
+// validateOwnershipTransfer performs additional security validations for ownership transfer
+func (s *EnvironmentService) validateOwnershipTransfer(
+	ctx context.Context,
+	req *environmentproto.UpdateOrganizationRequest,
+	editor *eventproto.Editor,
+	localizer locale.Localizer,
+) error {
+	// Get current organization to validate against
+	organization, err := s.orgStorage.GetOrganization(ctx, req.Id)
+	if err != nil {
+		return err
+	}
+
+	// Determine the new owner email being requested
+	var newOwnerEmail string
+	if req.OwnerEmail != nil {
+		newOwnerEmail = req.OwnerEmail.Value
+	} else if req.ChangeOwnerEmailCommand != nil {
+		newOwnerEmail = req.ChangeOwnerEmailCommand.OwnerEmail
+	}
+
+	// Don't allow no-op updates (setting same owner)
+	if newOwnerEmail == organization.OwnerEmail {
+		dt, err := statusNoCommand.WithDetails(&errdetails.LocalizedMessage{
+			Locale: localizer.GetLocale(),
+			Message: localizer.MustLocalizeWithTemplate(
+				locale.InvalidArgumentError,
+				"new owner email is the same as the current owner",
+			),
+		})
+		if err != nil {
+			return statusInternal.Err()
+		}
+		return dt.Err()
+	}
+
+	// If not system admin, ensure current user is actually the current owner
+	if !editor.IsAdmin && editor.Email != organization.OwnerEmail {
+		dt, err := statusPermissionDenied.WithDetails(&errdetails.LocalizedMessage{
+			Locale:  localizer.GetLocale(),
+			Message: localizer.MustLocalize(locale.PermissionDenied),
+		})
+		if err != nil {
+			return statusInternal.Err()
+		}
+		return dt.Err()
+	}
+
+	// New owner must exist and be a member of the organization
+	newOwnerAccount, err := s.accountStorage.GetAccountV2(ctx, newOwnerEmail, req.Id)
+	if err != nil {
+		if errors.Is(err, v2acc.ErrAccountNotFound) {
+			dt, err := statusNotFound.WithDetails(&errdetails.LocalizedMessage{
+				Locale:  localizer.GetLocale(),
+				Message: localizer.MustLocalizeWithTemplate(locale.NotFoundError, "new owner account not found in organization"),
+			})
+			if err != nil {
+				return statusInternal.Err()
+			}
+			return dt.Err()
+		}
+		return err
+	}
+
+	// New owner account must be enabled
+	if newOwnerAccount.Disabled {
+		dt, err := statusPermissionDenied.WithDetails(&errdetails.LocalizedMessage{
+			Locale:  localizer.GetLocale(),
+			Message: localizer.MustLocalizeWithTemplate(locale.InvalidArgumentError, "new owner account is disabled"),
+		})
+		if err != nil {
+			return statusInternal.Err()
+		}
+		return dt.Err()
+	}
+
+	return nil
+}
+
 func (s *EnvironmentService) updateOwnerRole(
 	ctx context.Context,
 	organizationID, prevOwnerEmail, newOwnerEmail string,
 ) error {
-	accStorage := v2acc.NewAccountStorage(s.mysqlClient)
 	// Update the old owner organization role
-	prevOwnerAcc, err := accStorage.GetAccountV2(ctx, prevOwnerEmail, organizationID)
+	prevOwnerAcc, err := s.accountStorage.GetAccountV2(ctx, prevOwnerEmail, organizationID)
 	if err != nil {
 		return err
 	}
 	if err := prevOwnerAcc.ChangeOrganizationRole(accountproto.AccountV2_Role_Organization_ADMIN); err != nil {
 		return err
 	}
-	if err := accStorage.UpdateAccountV2(ctx, prevOwnerAcc); err != nil {
+	if err := s.accountStorage.UpdateAccountV2(ctx, prevOwnerAcc); err != nil {
 		return err
 	}
 	// Update the new owner organization role
-	newOwnerAcc, err := accStorage.GetAccountV2(ctx, newOwnerEmail, organizationID)
+	newOwnerAcc, err := s.accountStorage.GetAccountV2(ctx, newOwnerEmail, organizationID)
 	if err != nil {
 		return err
 	}
 	if err := newOwnerAcc.ChangeOrganizationRole(accountproto.AccountV2_Role_Organization_OWNER); err != nil {
 		return err
 	}
-	if err := accStorage.UpdateAccountV2(ctx, newOwnerAcc); err != nil {
+	if err := s.accountStorage.UpdateAccountV2(ctx, newOwnerAcc); err != nil {
 		return err
 	}
 	return nil

pkg/environment/api/organization_test.go

@@ -14,6 +14,9 @@ import (
 	gstatus "google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/wrapperspb"
 
+	acmock "github.com/bucketeer-io/bucketeer/pkg/account/client/mock"
+	accountdomain "github.com/bucketeer-io/bucketeer/pkg/account/domain"
+	v2as "github.com/bucketeer-io/bucketeer/pkg/account/storage/v2"
 	accstoragemock "github.com/bucketeer-io/bucketeer/pkg/account/storage/v2/mock"
 	"github.com/bucketeer-io/bucketeer/pkg/environment/domain"
 	v2es "github.com/bucketeer-io/bucketeer/pkg/environment/storage/v2"
@@ -22,6 +25,7 @@ import (
 	publishermock "github.com/bucketeer-io/bucketeer/pkg/pubsub/publisher/mock"
 	"github.com/bucketeer-io/bucketeer/pkg/storage/v2/mysql"
 	mysqlmock "github.com/bucketeer-io/bucketeer/pkg/storage/v2/mysql/mock"
+	accountproto "github.com/bucketeer-io/bucketeer/proto/account"
 	proto "github.com/bucketeer-io/bucketeer/proto/environment"
 )
 
@@ -1264,3 +1268,213 @@ func TestEnvironmentService_CreateDemoOrganization(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateOwnershipTransfer(t *testing.T) {
+	t.Parallel()
+	mockController := gomock.NewController(t)
+	defer mockController.Finish()
+
+	// Create context with system admin token for most tests
+	ctxAdmin := createContextWithToken(t)
+	ctxAdmin = metadata.NewIncomingContext(ctxAdmin, metadata.MD{
+		"accept-language": []string{"ja"},
+	})
+
+	// Create context with non-admin token for ownership validation tests
+	ctxOwner := createContextWithTokenRoleUnassigned(t)
+	ctxOwner = metadata.NewIncomingContext(ctxOwner, metadata.MD{
+		"accept-language": []string{"ja"},
+	})
+
+	localizer := locale.NewLocalizer(ctxAdmin)
+	createError := func(status *gstatus.Status, msg string) error {
+		st, err := status.WithDetails(&errdetails.LocalizedMessage{
+			Locale:  localizer.GetLocale(),
+			Message: msg,
+		})
+		require.NoError(t, err)
+		return st.Err()
+	}
+
+	patterns := []struct {
+		desc        string
+		ctx         context.Context
+		setup       func(*EnvironmentService)
+		req         *proto.UpdateOrganizationRequest
+		expectedErr error
+	}{
+		{
+			desc: "success: no ownership transfer (name change only)",
+			ctx:  ctxAdmin,
+			setup: func(s *EnvironmentService) {
+				s.mysqlClient.(*mysqlmock.MockClient).EXPECT().RunInTransactionV2(
+					gomock.Any(), gomock.Any(),
+				).Return(nil)
+				s.publisher.(*publishermock.MockPublisher).EXPECT().Publish(gomock.Any(), gomock.Any()).Return(nil)
+			},
+			req: &proto.UpdateOrganizationRequest{
+				Id:   "org-1",
+				Name: wrapperspb.String("New Organization Name"),
+			},
+			expectedErr: nil,
+		},
+		{
+			desc: "err: no-op ownership transfer (same owner email)",
+			ctx:  ctxAdmin,
+			setup: func(s *EnvironmentService) {
+				s.orgStorage.(*storagemock.MockOrganizationStorage).EXPECT().GetOrganization(
+					gomock.Any(), "org-1",
+				).Return(&domain.Organization{
+					Organization: &proto.Organization{
+						Id:         "org-1",
+						OwnerEmail: "current-owner@example.com",
+					},
+				}, nil)
+			},
+			req: &proto.UpdateOrganizationRequest{
+				Id:         "org-1",
+				OwnerEmail: wrapperspb.String("current-owner@example.com"),
+			},
+			expectedErr: createError(statusNoCommand, localizer.MustLocalizeWithTemplate(
+				locale.InvalidArgumentError,
+				"new owner email is the same as the current owner",
+			)),
+		},
+		{
+			desc: "err: non-owner trying to transfer ownership",
+			ctx:  ctxOwner,
+			setup: func(s *EnvironmentService) {
+				s.accountClient.(*acmock.MockClient).EXPECT().GetAccountV2(
+					gomock.Any(), gomock.Any(),
+				).Return(&accountproto.GetAccountV2Response{
+					Account: &accountproto.AccountV2{
+						Email:            "email",
+						OrganizationRole: accountproto.AccountV2_Role_Organization_OWNER,
+					},
+				}, nil)
+				s.orgStorage.(*storagemock.MockOrganizationStorage).EXPECT().GetOrganization(
+					gomock.Any(), "org-1",
+				).Return(&domain.Organization{
+					Organization: &proto.Organization{
+						Id:         "org-1",
+						OwnerEmail: "current-owner@example.com", // Different from token email
+					},
+				}, nil)
+			},
+			req: &proto.UpdateOrganizationRequest{
+				Id:         "org-1",
+				OwnerEmail: wrapperspb.String("new-owner@example.com"),
+			},
+			expectedErr: createError(statusPermissionDenied, localizer.MustLocalize(locale.PermissionDenied)),
+		},
+		{
+			desc: "err: new owner account not found",
+			ctx:  ctxAdmin,
+			setup: func(s *EnvironmentService) {
+				s.orgStorage.(*storagemock.MockOrganizationStorage).EXPECT().GetOrganization(
+					gomock.Any(), "org-1",
+				).Return(&domain.Organization{
+					Organization: &proto.Organization{
+						Id:         "org-1",
+						OwnerEmail: "current-owner@example.com",
+					},
+				}, nil)
+				s.accountStorage.(*accstoragemock.MockAccountStorage).EXPECT().GetAccountV2(
+					gomock.Any(), "new-owner@example.com", "org-1",
+				).Return(nil, v2as.ErrAccountNotFound)
+			},
+			req: &proto.UpdateOrganizationRequest{
+				Id:         "org-1",
+				OwnerEmail: wrapperspb.String("new-owner@example.com"),
+			},
+			expectedErr: createError(statusNotFound, localizer.MustLocalizeWithTemplate(locale.NotFoundError, "new owner account not found in organization")),
+		},
+		{
+			desc: "err: new owner account is disabled",
+			ctx:  ctxAdmin,
+			setup: func(s *EnvironmentService) {
+				s.orgStorage.(*storagemock.MockOrganizationStorage).EXPECT().GetOrganization(
+					gomock.Any(), "org-1",
+				).Return(&domain.Organization{
+					Organization: &proto.Organization{
+						Id:         "org-1",
+						OwnerEmail: "current-owner@example.com",
+					},
+				}, nil)
+				s.accountStorage.(*accstoragemock.MockAccountStorage).EXPECT().GetAccountV2(
+					gomock.Any(), "new-owner@example.com", "org-1",
+				).Return(&accountdomain.AccountV2{
+					AccountV2: &accountproto.AccountV2{
+						Email:    "new-owner@example.com",
+						Disabled: true,
+					},
+				}, nil)
+			},
+			req: &proto.UpdateOrganizationRequest{
+				Id:         "org-1",
+				OwnerEmail: wrapperspb.String("new-owner@example.com"),
+			},
+			expectedErr: createError(statusPermissionDenied, localizer.MustLocalizeWithTemplate(locale.InvalidArgumentError, "new owner account is disabled")),
+		},
+		{
+			desc: "success: valid ownership transfer passes validation",
+			ctx:  ctxAdmin,
+			setup: func(s *EnvironmentService) {
+				// Mock validation phase - validateOwnershipTransfer
+				s.orgStorage.(*storagemock.MockOrganizationStorage).EXPECT().GetOrganization(
+					gomock.Any(), "org-1",
+				).Return(&domain.Organization{
+					Organization: &proto.Organization{
+						Id:         "org-1",
+						OwnerEmail: "current-owner@example.com",
+					},
+				}, nil)
+				s.accountStorage.(*accstoragemock.MockAccountStorage).EXPECT().GetAccountV2(
+					gomock.Any(), "new-owner@example.com", "org-1",
+				).Return(&accountdomain.AccountV2{
+					AccountV2: &accountproto.AccountV2{
+						Email:    "new-owner@example.com",
+						Disabled: false,
+					},
+				}, nil)
+
+				// Mock transaction execution (simplified)
+				s.mysqlClient.(*mysqlmock.MockClient).EXPECT().RunInTransactionV2(
+					gomock.Any(), gomock.Any(),
+				).Return(nil)
+				s.publisher.(*publishermock.MockPublisher).EXPECT().Publish(gomock.Any(), gomock.Any()).Return(nil)
+
+				// Mock updateOwnerRole calls (these happen after transaction)
+				s.accountStorage.(*accstoragemock.MockAccountStorage).EXPECT().GetAccountV2(
+					gomock.Any(), "current-owner@example.com", "org-1",
+				).Return(&accountdomain.AccountV2{
+					AccountV2: &accountproto.AccountV2{Email: "current-owner@example.com"},
+				}, nil).AnyTimes()
+				s.accountStorage.(*accstoragemock.MockAccountStorage).EXPECT().UpdateAccountV2(
+					gomock.Any(), gomock.Any(),
+				).Return(nil).AnyTimes()
+				s.accountStorage.(*accstoragemock.MockAccountStorage).EXPECT().GetAccountV2(
+					gomock.Any(), "new-owner@example.com", "org-1",
+				).Return(&accountdomain.AccountV2{
+					AccountV2: &accountproto.AccountV2{Email: "new-owner@example.com"},
+				}, nil).AnyTimes()
+			},
+			req: &proto.UpdateOrganizationRequest{
+				Id:         "org-1",
+				OwnerEmail: wrapperspb.String("new-owner@example.com"),
+			},
+			expectedErr: nil,
+		},
+	}
+
+	for _, p := range patterns {
+		t.Run(p.desc, func(t *testing.T) {
+			service := newEnvironmentService(t, mockController, nil)
+			if p.setup != nil {
+				p.setup(service)
+			}
+			_, err := service.UpdateOrganization(p.ctx, p.req)
+			assert.Equal(t, p.expectedErr, err)
+		})
+	}
+}