diff --git a/internal/api/v1beta1connect/permission_check.go b/internal/api/v1beta1connect/permission_check.go index 352b81b46..bc19bac10 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()) @@ -162,13 +162,8 @@ 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 == "" { - 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, req.Msg.GetPermission()) @@ -201,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 7cc281a2b..4cfb080d0 100644 --- a/internal/api/v1beta1connect/permission_check_test.go +++ b/internal/api/v1beta1connect/permission_check_test.go @@ -33,12 +33,48 @@ 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), + wantErr: connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation), + }, + { + 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, 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", @@ -91,9 +127,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 +148,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, @@ -138,3 +172,47 @@ 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), + }, + { + 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) { + mockDep := &ConnectHandler{} + resp, err := mockDep.BatchCheckPermission(context.Background(), tt.request) + assert.Equal(t, tt.wantErr, err) + assert.Nil(t, resp) + }) + } +} diff --git a/test/e2e/regression/api_test.go b/test/e2e/regression/api_test.go index 191458f81..c8c3d5afd 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 501786ed6..2e4c033ca 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 6850c2e0e..5395299fa 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())