diff --git a/assets/builtin-policy.csv b/assets/builtin-policy.csv
index 28f565260d88d..088f5fbd08ad3 100644
--- a/assets/builtin-policy.csv
+++ b/assets/builtin-policy.csv
@@ -18,7 +18,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
@@ -47,4 +49,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 9b7775a65e3e5..0b485d51ce749 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 `<action>` will have the `<action>/<group>/<kind>/<ns>/<name>` format.
@@ -148,15 +148,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 438e9c34064f1..0979517b4e73e 100644
--- a/server/application/application.go
+++ b/server/application/application.go
@@ -1346,12 +1346,10 @@ func (s *Server) getAppResources(ctx context.Context, a *v1alpha1.Application) (
}
func (s *Server) getAppLiveResource(ctx context.Context, action string, q *application.ApplicationResourceRequest) (*v1alpha1.ResourceNode, *rest.Config, *v1alpha1.Application, error) {
- a, _, err := s.getApplicationEnforceRBACInformer(ctx, action, q.GetProject(), q.GetAppNamespace(), q.GetName())
- if err != nil && errors.Is(err, argocommon.PermissionDeniedAPIError) && (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 b1a4d88ec082c..ad2b1f04960d3 100644
--- a/server/application/application_test.go
+++ b/server/application/application_test.go
@@ -1652,7 +1652,7 @@ func TestDeleteResourcesRBAC(t *testing.T) {
p, test-user, applications, delete, default/test-app, allow
`)
_, err := appServer.DeleteResource(ctx, &req)
- assert.EqualError(t, err, expectedErrorWhenDeleteAllowed)
+ assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
})
t.Run("delete with application permission but deny subresource", func(t *testing.T) {
@@ -1661,7 +1661,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.EqualError(t, err, expectedErrorWhenDeleteAllowed)
+ assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
})
t.Run("delete with subresource", func(t *testing.T) {
@@ -1715,7 +1715,7 @@ func TestPatchResourcesRBAC(t *testing.T) {
p, test-user, applications, update, default/test-app, allow
`)
_, err := appServer.PatchResource(ctx, &req)
- assert.EqualError(t, err, expectedErrorWhenUpdateAllowed)
+ assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
})
t.Run("patch with application permission but deny subresource", func(t *testing.T) {
@@ -1724,7 +1724,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.EqualError(t, err, expectedErrorWhenUpdateAllowed)
+ assert.Equal(t, codes.PermissionDenied.String(), status.Code(err).String())
})
t.Run("patch with subresource", func(t *testing.T) {
@@ -1754,6 +1754,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)