From 71057102f7dad811fcbd0105f5354399aa0e4437 Mon Sep 17 00:00:00 2001 From: aman Date: Fri, 14 Aug 2026 15:42:37 +0530 Subject: [PATCH 1/3] refactor(api): require resource in CheckResourcePermission CheckResourcePermission reads the object only from the resource field ("namespace:id") and returns InvalidArgument when it is missing or malformed. The deprecated object_id/object_namespace request fields are no longer read. E2E tests send the resource form. Co-Authored-By: Claude Fable 5 --- .../api/v1beta1connect/permission_check.go | 7 +---- .../v1beta1connect/permission_check_test.go | 20 ++++++++----- test/e2e/regression/api_test.go | 5 ++-- test/e2e/regression/onboarding_test.go | 30 ++++++++----------- test/e2e/regression/serviceusers_test.go | 10 +++---- 5 files changed, 32 insertions(+), 40 deletions(-) diff --git a/internal/api/v1beta1connect/permission_check.go b/internal/api/v1beta1connect/permission_check.go index 352b81b467..6d58649ab7 100644 --- a/internal/api/v1beta1connect/permission_check.go +++ b/internal/api/v1beta1connect/permission_check.go @@ -162,12 +162,7 @@ func (h *ConnectHandler) CheckResourcePermission(ctx context.Context, req *conne errorLogger := NewErrorLogger() objectNamespace, objectID, err := schema.SplitNamespaceAndResourceID(req.Msg.GetResource()) - //nolint:staticcheck - if len(req.Msg.GetResource()) == 0 || err != nil { - objectNamespace = schema.ParseNamespaceAliasIfRequired(req.Msg.GetObjectNamespace()) - objectID = req.Msg.GetObjectId() - } - if objectNamespace == "" || objectID == "" { + if err != nil || objectNamespace == "" || objectID == "" { return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) } diff --git a/internal/api/v1beta1connect/permission_check_test.go b/internal/api/v1beta1connect/permission_check_test.go index 7cc281a2b8..b31e373212 100644 --- a/internal/api/v1beta1connect/permission_check_test.go +++ b/internal/api/v1beta1connect/permission_check_test.go @@ -33,13 +33,21 @@ func TestHandler_CheckResourcePermission(t *testing.T) { wantErr error }{ { - name: "should return bad request error if object id is empty or namespace is empty", + name: "should return bad request error if resource is malformed", request: connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ Resource: "not-namespace-uuid-format", }), want: nil, wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest), }, + { + name: "should return bad request error if resource is missing", + request: connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ + Permission: schema.UpdatePermission, + }), + want: nil, + wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest), + }, { name: "should return user unauthenticated error if CheckAuthz function returns ErrUnauthenticated", setup: func(res *mocks.ResourceService, perm *mocks.PermissionService) { @@ -91,9 +99,8 @@ func TestHandler_CheckResourcePermission(t *testing.T) { Return(testPermission, nil) }, request: connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ - ObjectId: testRelationV2.Object.ID, - ObjectNamespace: testRelationV2.Object.Namespace, - Permission: schema.UpdatePermission, + Permission: schema.UpdatePermission, + Resource: schema.JoinNamespaceAndResourceID(testRelationV2.Object.Namespace, testRelationV2.Object.ID), }), want: connect.NewResponse(&frontierv1beta1.CheckResourcePermissionResponse{ Status: true, @@ -113,9 +120,8 @@ func TestHandler_CheckResourcePermission(t *testing.T) { Return(testPermission, nil) }, request: connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ - ObjectId: testRelationV2.Object.ID, - ObjectNamespace: testRelationV2.Object.Namespace, - Permission: schema.UpdatePermission, + Permission: schema.UpdatePermission, + Resource: schema.JoinNamespaceAndResourceID(testRelationV2.Object.Namespace, testRelationV2.Object.ID), }), want: connect.NewResponse(&frontierv1beta1.CheckResourcePermissionResponse{ Status: false, diff --git a/test/e2e/regression/api_test.go b/test/e2e/regression/api_test.go index 191458f81c..c8c3d5afdb 100644 --- a/test/e2e/regression/api_test.go +++ b/test/e2e/regression/api_test.go @@ -1676,9 +1676,8 @@ func (s *APIRegressionTestSuite) TestRelationAPI() { s.Assert().Equal(true, checkViewPermResp.Msg.GetStatus()) checkEditPermResp, err := s.testBench.Client.CheckResourcePermission(ctxOrgUserAuth, connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ - ObjectId: existingOrg.Msg.GetOrganization().GetId(), - ObjectNamespace: schema.OrganizationNamespace, - Permission: schema.UpdatePermission, + Resource: schema.JoinNamespaceAndResourceID(schema.OrganizationNamespace, existingOrg.Msg.GetOrganization().GetId()), + Permission: schema.UpdatePermission, })) s.Assert().NoError(err) s.Assert().Equal(true, checkEditPermResp.Msg.GetStatus()) diff --git a/test/e2e/regression/onboarding_test.go b/test/e2e/regression/onboarding_test.go index 501786ed61..2e4c033ca1 100644 --- a/test/e2e/regression/onboarding_test.go +++ b/test/e2e/regression/onboarding_test.go @@ -156,9 +156,8 @@ func (s *OnboardingRegressionTestSuite) TestOnboardOrganizationWithUser() { }) s.Run("4. org admin should have access to the resource created", func() { createResourceResp, err := s.testBench.Client.CheckResourcePermission(ctx, connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ - ObjectId: resourceID, - ObjectNamespace: computeOrderNamespace, - Permission: schema.UpdatePermission, + Resource: schema.JoinNamespaceAndResourceID(computeOrderNamespace, resourceID), + Permission: schema.UpdatePermission, })) s.Assert().NoError(err) s.Assert().NotNil(createResourceResp) @@ -245,9 +244,8 @@ func (s *OnboardingRegressionTestSuite) TestOnboardOrganizationWithUser() { userCtx := testbench.ContextWithAuth(context.Background(), newUserCookie) checkUpdateProjectResp, err := s.testBench.Client.CheckResourcePermission(userCtx, connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ - ObjectId: projectID, - ObjectNamespace: schema.ProjectNamespace, - Permission: schema.UpdatePermission, + Resource: schema.JoinNamespaceAndResourceID(schema.ProjectNamespace, projectID), + Permission: schema.UpdatePermission, })) s.Assert().NoError(err) s.Assert().NotNil(checkUpdateProjectResp) @@ -255,9 +253,8 @@ func (s *OnboardingRegressionTestSuite) TestOnboardOrganizationWithUser() { // resources under the project checkUpdateResourceResp, err := s.testBench.Client.CheckResourcePermission(userCtx, connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ - ObjectId: resourceID, - ObjectNamespace: computeOrderNamespace, - Permission: schema.UpdatePermission, + Resource: schema.JoinNamespaceAndResourceID(computeOrderNamespace, resourceID), + Permission: schema.UpdatePermission, })) s.Assert().NoError(err) s.Assert().NotNil(checkUpdateResourceResp) @@ -269,9 +266,8 @@ func (s *OnboardingRegressionTestSuite) TestOnboardOrganizationWithUser() { userCtx := testbench.ContextWithAuth(context.Background(), newUserCookie) checkUpdateOrgResp, err := s.testBench.Client.CheckResourcePermission(userCtx, connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ - ObjectId: orgID, - ObjectNamespace: schema.OrganizationNamespace, - Permission: schema.UpdatePermission, + Resource: schema.JoinNamespaceAndResourceID(schema.OrganizationNamespace, orgID), + Permission: schema.UpdatePermission, })) s.Assert().NoError(err) s.Assert().NotNil(checkUpdateOrgResp) @@ -323,18 +319,16 @@ func (s *OnboardingRegressionTestSuite) TestOnboardOrganizationWithUser() { userCtx := testbench.ContextWithAuth(context.Background(), newUserCookie) checkGetResourceResp, err := s.testBench.Client.CheckResourcePermission(userCtx, connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ - ObjectId: resourceID, - ObjectNamespace: computeOrderNamespace, - Permission: schema.GetPermission, + Resource: schema.JoinNamespaceAndResourceID(computeOrderNamespace, resourceID), + Permission: schema.GetPermission, })) s.Assert().NoError(err) s.Assert().NotNil(checkGetResourceResp) s.Assert().True(checkGetResourceResp.Msg.GetStatus()) checkUpdateResourceResp, err := s.testBench.Client.CheckResourcePermission(userCtx, connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ - ObjectId: resourceID, - ObjectNamespace: computeOrderNamespace, - Permission: schema.UpdatePermission, + Resource: schema.JoinNamespaceAndResourceID(computeOrderNamespace, resourceID), + Permission: schema.UpdatePermission, })) s.Assert().NoError(err) s.Assert().NotNil(checkUpdateResourceResp) diff --git a/test/e2e/regression/serviceusers_test.go b/test/e2e/regression/serviceusers_test.go index 6850c2e0e5..5395299fa9 100644 --- a/test/e2e/regression/serviceusers_test.go +++ b/test/e2e/regression/serviceusers_test.go @@ -221,9 +221,8 @@ func (s *ServiceUsersRegressionTestSuite) TestServiceUserWithKey() { s.Assert().NoError(err) checkPermAfterResp, err := s.testBench.Client.CheckResourcePermission(ctxWithKey, connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ - ObjectId: existingOrg.Msg.GetOrganization().GetId(), - ObjectNamespace: "organization", - Permission: schema.UpdatePermission, + Resource: schema.JoinNamespaceAndResourceID("organization", existingOrg.Msg.GetOrganization().GetId()), + Permission: schema.UpdatePermission, })) s.Assert().NoError(err) s.Assert().True(checkPermAfterResp.Msg.GetStatus()) @@ -526,9 +525,8 @@ func (s *ServiceUsersRegressionTestSuite) TestServiceUserWithSecret() { s.Assert().NoError(err) checkPermAfterResp, err := s.testBench.Client.CheckResourcePermission(ctxWithKey, connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ - ObjectId: existingOrg.Msg.GetOrganization().GetId(), - ObjectNamespace: "organization", - Permission: schema.ProjectCreatePermission, + Resource: schema.JoinNamespaceAndResourceID("organization", existingOrg.Msg.GetOrganization().GetId()), + Permission: schema.ProjectCreatePermission, })) s.Assert().NoError(err) s.Assert().True(checkPermAfterResp.Msg.GetStatus()) From 27ee7b2235cd8d99ac621f6b2d04ae3260167664 Mon Sep 17 00:00:00 2001 From: aman Date: Mon, 17 Aug 2026 12:14:29 +0530 Subject: [PATCH 2/3] refactor(api): align resource guards across check endpoints BatchCheckPermission rejects a resource with an empty namespace or id as InvalidArgument instead of failing at SpiceDB as internal. All three resource guards in the file, and the federated subject guard, return the namespace-notation error so callers see the expected format. Co-Authored-By: Claude Fable 5 --- .../api/v1beta1connect/permission_check.go | 10 +-- .../v1beta1connect/permission_check_test.go | 67 ++++++++++++++++++- 2 files changed, 70 insertions(+), 7 deletions(-) diff --git a/internal/api/v1beta1connect/permission_check.go b/internal/api/v1beta1connect/permission_check.go index 6d58649ab7..bc19bac106 100644 --- a/internal/api/v1beta1connect/permission_check.go +++ b/internal/api/v1beta1connect/permission_check.go @@ -72,12 +72,12 @@ func (h *ConnectHandler) CheckFederatedResourcePermission(ctx context.Context, r objectNamespace, objectID, err := schema.SplitNamespaceAndResourceID(req.Msg.GetResource()) if err != nil || objectNamespace == "" || objectID == "" { - return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) + return nil, connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation) } principalNamespace, principalID, err := schema.SplitNamespaceAndResourceID(req.Msg.GetSubject()) if err != nil || principalNamespace == "" || principalID == "" { - return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) + return nil, connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation) } permissionName, err := h.getPermissionName(ctx, objectNamespace, req.Msg.GetPermission()) @@ -163,7 +163,7 @@ func (h *ConnectHandler) CheckResourcePermission(ctx context.Context, req *conne objectNamespace, objectID, err := schema.SplitNamespaceAndResourceID(req.Msg.GetResource()) if err != nil || objectNamespace == "" || objectID == "" { - return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) + return nil, connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation) } permissionName, err := h.getPermissionName(ctx, objectNamespace, req.Msg.GetPermission()) @@ -196,8 +196,8 @@ func (h *ConnectHandler) BatchCheckPermission(ctx context.Context, req *connect. checks := make([]resource.Check, 0, len(req.Msg.GetBodies())) for _, body := range req.Msg.GetBodies() { objectNamespace, objectID, err := schema.SplitNamespaceAndResourceID(body.GetResource()) - if len(body.GetResource()) == 0 || err != nil { - return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) + if err != nil || objectNamespace == "" || objectID == "" { + return nil, connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation) } permissionName, err := h.getPermissionName(ctx, objectNamespace, body.GetPermission()) diff --git a/internal/api/v1beta1connect/permission_check_test.go b/internal/api/v1beta1connect/permission_check_test.go index b31e373212..01c510139d 100644 --- a/internal/api/v1beta1connect/permission_check_test.go +++ b/internal/api/v1beta1connect/permission_check_test.go @@ -38,7 +38,7 @@ func TestHandler_CheckResourcePermission(t *testing.T) { Resource: "not-namespace-uuid-format", }), want: nil, - wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest), + wantErr: connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation), }, { name: "should return bad request error if resource is missing", @@ -46,7 +46,35 @@ func TestHandler_CheckResourcePermission(t *testing.T) { Permission: schema.UpdatePermission, }), want: nil, - wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest), + wantErr: connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation), + }, + { + name: "should return bad request error if resource id part is empty", + request: connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ + Resource: "organization:", + Permission: schema.UpdatePermission, + }), + want: nil, + wantErr: connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation), + }, + { + name: "should return bad request error if resource namespace part is empty", + request: connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ + Resource: ":" + testRelationV2.Object.ID, + Permission: schema.UpdatePermission, + }), + want: nil, + wantErr: connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation), + }, + { + name: "should return bad request error if only the removed split fields are sent", + request: connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ + ObjectId: testRelationV2.Object.ID, + ObjectNamespace: testRelationV2.Object.Namespace, + Permission: schema.UpdatePermission, + }), + want: nil, + wantErr: connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation), }, { name: "should return user unauthenticated error if CheckAuthz function returns ErrUnauthenticated", @@ -144,3 +172,38 @@ func TestHandler_CheckResourcePermission(t *testing.T) { }) } } + +func TestHandler_BatchCheckPermission(t *testing.T) { + tests := []struct { + name string + request *connect.Request[frontierv1beta1.BatchCheckPermissionRequest] + wantErr error + }{ + { + name: "should return bad request error if a body resource is malformed", + request: connect.NewRequest(&frontierv1beta1.BatchCheckPermissionRequest{ + Bodies: []*frontierv1beta1.BatchCheckPermissionBody{ + {Resource: "not-namespace-uuid-format", Permission: schema.UpdatePermission}, + }, + }), + wantErr: connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation), + }, + { + name: "should return bad request error if a body resource id part is empty", + request: connect.NewRequest(&frontierv1beta1.BatchCheckPermissionRequest{ + Bodies: []*frontierv1beta1.BatchCheckPermissionBody{ + {Resource: "organization:", Permission: schema.UpdatePermission}, + }, + }), + wantErr: connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockDep := &ConnectHandler{} + resp, err := mockDep.BatchCheckPermission(context.Background(), tt.request) + assert.Equal(t, tt.wantErr, err) + assert.Nil(t, resp) + }) + } +} From e5241f57e2153bfa0f798b3de7438d59bcc0c2a5 Mon Sep 17 00:00:00 2001 From: aman Date: Mon, 17 Aug 2026 19:54:11 +0530 Subject: [PATCH 3/3] test(api): cover the empty namespace part in batch check bodies Co-Authored-By: Claude Fable 5 --- internal/api/v1beta1connect/permission_check_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/internal/api/v1beta1connect/permission_check_test.go b/internal/api/v1beta1connect/permission_check_test.go index 01c510139d..4cfb080d0f 100644 --- a/internal/api/v1beta1connect/permission_check_test.go +++ b/internal/api/v1beta1connect/permission_check_test.go @@ -197,6 +197,15 @@ func TestHandler_BatchCheckPermission(t *testing.T) { }), wantErr: connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation), }, + { + name: "should return bad request error if a body resource namespace part is empty", + request: connect.NewRequest(&frontierv1beta1.BatchCheckPermissionRequest{ + Bodies: []*frontierv1beta1.BatchCheckPermissionBody{ + {Resource: ":" + testRelationV2.Object.ID, Permission: schema.UpdatePermission}, + }, + }), + wantErr: connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation), + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {