From 71057102f7dad811fcbd0105f5354399aa0e4437 Mon Sep 17 00:00:00 2001 From: aman Date: Fri, 14 Aug 2026 15:42:37 +0530 Subject: [PATCH] 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())