diff --git a/assets/builtin-policy.csv b/assets/builtin-policy.csv index 81c8ca5092cb4..8c0cf2c25bfb1 100644 --- a/assets/builtin-policy.csv +++ b/assets/builtin-policy.csv @@ -17,7 +17,9 @@ p, role:readonly, logs, get, */*, allow p, role:admin, applications, create, */*, allow p, role:admin, applications, update, */*, allow +p, role:admin, applications, update/*, */*, allow p, role:admin, applications, delete, */*, allow +p, role:admin, applications, delete/*, */*, allow p, role:admin, applications, sync, */*, allow p, role:admin, applications, override, */*, allow p, role:admin, applications, action/*, */*, allow @@ -43,4 +45,4 @@ p, role:admin, gpgkeys, delete, *, allow p, role:admin, exec, create, */*, allow g, role:admin, role:readonly -g, admin, role:admin \ No newline at end of file +g, admin, role:admin diff --git a/docs/operator-manual/rbac.md b/docs/operator-manual/rbac.md index e85be535bd826..e7ad426a85955 100644 --- a/docs/operator-manual/rbac.md +++ b/docs/operator-manual/rbac.md @@ -114,7 +114,7 @@ The `applications` resource is an [Application-Specific Policy](#application-spe #### Fine-grained Permissions for `update`/`delete` action -The `update` and `delete` actions, when granted on an application, will allow the user to perform the operation on the application itself **and** all of its resources. +The `update` and `delete` actions, when granted on an application, will allow the user to perform the operation on the application itself but not its resources. It can be desirable to only allow `update` or `delete` on specific resources within an application. To do so, when the action if performed on an application's resource, the `` will have the `////` format. @@ -138,15 +138,12 @@ p, example-user, applications, delete, default/prod-app, deny p, example-user, applications, delete/*/Pod/*, default/prod-app, allow ``` -!!! note - - It is not possible to deny fine-grained permissions for a sub-resource if the action was **explicitly allowed on the application**. - For instance, the following policies will **allow** a user to delete the Pod and any other resources in the application: +If we want to explicitly allow updates to the application, but deny updates to any sub-resources: - ```csv - p, example-user, applications, delete, default/prod-app, allow - p, example-user, applications, delete/*/Pod/*, default/prod-app, deny - ``` +```csv +p, example-user, applications, update, default/prod-app, allow +p, example-user, applications, update/*, default/prod-app, deny +``` #### The `action` action diff --git a/server/application/application.go b/server/application/application.go index de961c82ea5f7..f6cad80b542f1 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -1333,12 +1333,10 @@ func (s *Server) getAppResources(ctx context.Context, a *appv1.Application) (*ap } func (s *Server) getAppLiveResource(ctx context.Context, action string, q *application.ApplicationResourceRequest) (*appv1.ResourceNode, *rest.Config, *appv1.Application, error) { - a, _, err := s.getApplicationEnforceRBACInformer(ctx, action, q.GetProject(), q.GetAppNamespace(), q.GetName()) - if err != nil && errors.Is(err, permissionDeniedErr) && (action == rbacpolicy.ActionDelete || action == rbacpolicy.ActionUpdate) { - // If users dont have permission on the whole applications, maybe they have fine-grained access to the specific resources + if action == rbacpolicy.ActionDelete || action == rbacpolicy.ActionUpdate { action = fmt.Sprintf("%s/%s/%s/%s/%s", action, q.GetGroup(), q.GetKind(), q.GetNamespace(), q.GetResourceName()) - a, _, err = s.getApplicationEnforceRBACInformer(ctx, action, q.GetProject(), q.GetAppNamespace(), q.GetName()) } + a, _, err := s.getApplicationEnforceRBACInformer(ctx, action, q.GetProject(), q.GetAppNamespace(), q.GetName()) if err != nil { return nil, nil, nil, err } diff --git a/server/application/application_test.go b/server/application/application_test.go index a030131f679a9..3b579d8e9b142 100644 --- a/server/application/application_test.go +++ b/server/application/application_test.go @@ -1669,7 +1669,7 @@ func TestDeleteResourcesRBAC(t *testing.T) { p, test-user, applications, delete, default/test-app, allow `) _, err := appServer.DeleteResource(ctx, &req) - assert.Equal(t, expectedErrorWhenDeleteAllowed, err.Error()) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) }) t.Run("delete with application permission but deny subresource", func(t *testing.T) { @@ -1678,7 +1678,7 @@ p, test-user, applications, delete, default/test-app, allow p, test-user, applications, delete/*, default/test-app, deny `) _, err := appServer.DeleteResource(ctx, &req) - assert.Equal(t, expectedErrorWhenDeleteAllowed, err.Error()) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) }) t.Run("delete with subresource", func(t *testing.T) { @@ -1732,7 +1732,7 @@ func TestPatchResourcesRBAC(t *testing.T) { p, test-user, applications, update, default/test-app, allow `) _, err := appServer.PatchResource(ctx, &req) - assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error()) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) }) t.Run("patch with application permission but deny subresource", func(t *testing.T) { @@ -1741,7 +1741,7 @@ p, test-user, applications, update, default/test-app, allow p, test-user, applications, update/*, default/test-app, deny `) _, err := appServer.PatchResource(ctx, &req) - assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error()) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) }) t.Run("patch with subresource", func(t *testing.T) { @@ -1771,6 +1771,157 @@ p, test-user, applications, update/fake.io/PodTest/*, default/test-app, deny }) } +func TestUpdateApplicationRBAC(t *testing.T) { + ctx := context.Background() + // nolint:staticcheck + ctx = context.WithValue(ctx, "claims", &jwt.RegisteredClaims{Subject: "test-user"}) + testApp := newTestApp() + appServer := newTestAppServer(t, testApp) + appServer.enf.SetDefaultRole("") + testApp.Spec.Project = "" + + appSpecReq := application.ApplicationUpdateSpecRequest{ + Name: &testApp.Name, + AppNamespace: &testApp.Namespace, + Spec: &testApp.Spec, + } + + t.Run("can update application spec with update allow", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, */*, allow +`) + _, err := appServer.UpdateSpec(ctx, &appSpecReq) + require.NoError(t, err) + }) + + t.Run("cannot update application spec with sub-resource update allow", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update/*, default/test-app, allow +`) + _, err := appServer.UpdateSpec(ctx, &appSpecReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + }) + + resourceReq := application.ApplicationResourcePatchRequest{ + Name: &testApp.Name, + AppNamespace: &testApp.Namespace, + Group: strToPtr("fake.io"), + Kind: strToPtr("PodTest"), + Namespace: strToPtr("fake-ns"), + ResourceName: strToPtr("my-pod-test"), + } + + expectedErrorWhenUpdateAllowed := "rpc error: code = InvalidArgument desc = PodTest fake.io my-pod-test not found as part of application test-app" + + t.Run("can update application spec with update allow, sub-resource update deny", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, allow +p, test-user, applications, update/fake.io/PodTest/*, default/test-app, deny +`) + _, err := appServer.UpdateSpec(ctx, &appSpecReq) + require.NoError(t, err) + _, err = appServer.PatchResource(ctx, &resourceReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + }) + + t.Run("can update application spec with update allow, sub-resource update allow", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, allow +p, test-user, applications, update/fake.io/PodTest/*, default/test-app, allow +`) + _, err := appServer.UpdateSpec(ctx, &appSpecReq) + require.NoError(t, err) + _, err = appServer.PatchResource(ctx, &resourceReq) + assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error()) + }) + + t.Run("cannot update application spec with update deny, sub-resource update allow", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, deny +p, test-user, applications, update/fake.io/PodTest/*, default/test-app, allow +`) + _, err := appServer.UpdateSpec(ctx, &appSpecReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + _, err = appServer.PatchResource(ctx, &resourceReq) + assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error()) + }) + + t.Run("cannot update application spec with update deny, sub-resource update deny", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, deny +p, test-user, applications, update/fake.io/PodTest/*, default/test-app, deny +`) + _, err := appServer.UpdateSpec(ctx, &appSpecReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + _, err = appServer.PatchResource(ctx, &resourceReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + }) + + appReq := application.ApplicationUpdateRequest{ + Application: testApp, + } + + t.Run("update application with generic permission", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, allow +`) + _, err := appServer.Update(ctx, &appReq) + require.NoError(t, err) + }) + + t.Run("cannot update application with sub-resource permission", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update/*, default/test-app, allow +`) + _, err := appServer.Update(ctx, &appReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + }) + + t.Run("can update application with update allow, sub-resource update deny", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, allow +p, test-user, applications, update/fake.io/PodTest/*, default/test-app, deny +`) + _, err := appServer.Update(ctx, &appReq) + require.NoError(t, err) + _, err = appServer.PatchResource(ctx, &resourceReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + }) + + t.Run("can update application with update allow, sub-resource update allow", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, allow +p, test-user, applications, update/fake.io/PodTest/*, default/test-app, allow +`) + _, err := appServer.Update(ctx, &appReq) + require.NoError(t, err) + _, err = appServer.PatchResource(ctx, &resourceReq) + assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error()) + }) + + t.Run("cannot update application with update deny, sub-resource update allow", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, deny +p, test-user, applications, update/fake.io/PodTest/*, default/test-app, allow +`) + _, err := appServer.Update(ctx, &appReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + _, err = appServer.PatchResource(ctx, &resourceReq) + assert.Equal(t, expectedErrorWhenUpdateAllowed, err.Error()) + }) + + t.Run("cannot update application with update deny, sub-resource update deny", func(t *testing.T) { + _ = appServer.enf.SetBuiltinPolicy(` +p, test-user, applications, update, default/test-app, deny +p, test-user, applications, update/fake.io/PodTest/*, default/test-app, deny +`) + _, err := appServer.Update(ctx, &appReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + _, err = appServer.PatchResource(ctx, &resourceReq) + assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String()) + }) +} + func TestSyncAndTerminate(t *testing.T) { ctx := context.Background() appServer := newTestAppServer(t)