From 9aed5e093abd903c41a5b9edec5ab0decbf42e9c Mon Sep 17 00:00:00 2001
From: Andrew Ballantyne <8126518+andrewballantyne@users.noreply.github.com>
Date: Mon, 27 Jan 2025 13:20:04 -0500
Subject: [PATCH] Clarify and update permissions (#1538)

* Add readme to explain values of permissions

* NIM Accounts are namespaced resources

* Update permissions to fulfill doc design

* Based on feedback, less hard-fast ruling wording in the README
---
 controllers/services/auth/resources/README.md | 27 +++++++++++++++++++
 .../admingroup-clusterrole.tmpl.yaml          | 12 ---------
 .../auth/resources/admingroup-role.tmpl.yaml  | 24 ++++++++++++++++-
 3 files changed, 50 insertions(+), 13 deletions(-)
 create mode 100644 controllers/services/auth/resources/README.md

diff --git a/controllers/services/auth/resources/README.md b/controllers/services/auth/resources/README.md
new file mode 100644
index 00000000000..d04c710e697
--- /dev/null
+++ b/controllers/services/auth/resources/README.md
@@ -0,0 +1,27 @@
+# Permissions For Users
+
+When adding permissions for users, it's best to consider the "[reasonable level](#reasonable-level)" of permissions.
+
+## Reasonable Level
+
+> **Note:** This is a recommendation -- a good starting spot -- not a hard-fast rule.
+
+When granting access to a bot (like a service account) you'd aim to grant specific actions the bot would directly do -- need patch? give patch; need read? give read. However, when granting permissions to users, you often want to take a wider approach -- need patch? give update & patch; need read? give get, list, and watch.
+
+Admins should be considered as people with a desire of a clean user experience, and not a means to an end to complete a development flow. From there, we can look at if it is appropriate for us to be granting such wide permissions.
+
+Consider the CRUD actions an admin or allowed user gets... as a user is granted access over resources, there a logical good UX extension to grant them inverse or complementary access. Consider the following CRUD layout as it is related to each k8s verbs:
+* C - Create
+* R - Get, List, Watch
+* U - Update, Patch
+* D - Delete
+
+Furthermore, when a user gets “C” (create) actions... we should consider them having the ability to delete them, as to clean up their mistakes. Naturally they’ll need read permissions too so they can interact with them. The mapping looks something like this:
+* Need **C** => Gets **CRUD**
+* Need **R** => Gets **R**
+* Need **U** => Gets **RU**
+* Need **D** => Gets **CRUD**
+
+The guidance would be for you to start with the idea that if you need one of the CRUD actions, you get the corresponding CRUD relationship. It’s all additive. If you need just “R” – you only get to read (which boils down to get, list, & watch powers). If you need Create? You’d look to get the full CRUD gambit.
+
+It’s important to note, this is a guidance and recommendation for the user experience of the admin behind the permissions. Not a hard-fast rule. Consider the ramifications of broadly granting access to the resource in question you’re looking at.
diff --git a/controllers/services/auth/resources/admingroup-clusterrole.tmpl.yaml b/controllers/services/auth/resources/admingroup-clusterrole.tmpl.yaml
index 402673cbd56..4ab79189b4c 100644
--- a/controllers/services/auth/resources/admingroup-clusterrole.tmpl.yaml
+++ b/controllers/services/auth/resources/admingroup-clusterrole.tmpl.yaml
@@ -46,15 +46,3 @@ rules:
   - get
   - list
   - watch
-- apiGroups:
-  - nim.opendatahub.io
-  resources:
-  - accounts
-  verbs:
-  - watch
-  - update 
-  - get
-  - list
-  - create
-  - patch
-  - delete
diff --git a/controllers/services/auth/resources/admingroup-role.tmpl.yaml b/controllers/services/auth/resources/admingroup-role.tmpl.yaml
index fce1ef797ca..a35a24b040e 100644
--- a/controllers/services/auth/resources/admingroup-role.tmpl.yaml
+++ b/controllers/services/auth/resources/admingroup-role.tmpl.yaml
@@ -13,6 +13,7 @@ rules:
   - list
   - watch
   - patch
+  - update
 - apiGroups:
   - services.opendatahub.io
   resources:
@@ -27,7 +28,9 @@ rules:
   - create
   - get
   - list
+  - watch
   - patch
+  - update
   - delete
 - apiGroups:
   - route.openshift.io
@@ -54,6 +57,7 @@ rules:
   - get
   - list
   - patch
+  - update
   - delete
   - watch
 - apiGroups:
@@ -63,6 +67,8 @@ rules:
   - buildconfigs
   verbs:
   - list
+  - get
+  - watch
 - apiGroups:
   - apps
   resources:
@@ -76,6 +82,7 @@ rules:
   - odhdashboardconfigs
   verbs:
   - get
+  - list
   - watch
   - create
   - update
@@ -96,7 +103,8 @@ rules:
   - odhdocuments
   verbs:
   - get
-  - list      
+  - list
+  - watch
 - apiGroups:
   - console.openshift.io
   resources:
@@ -104,6 +112,7 @@ rules:
   verbs:
   - get
   - list
+  - watch
 - apiGroups:
   - template.openshift.io
   resources:
@@ -114,6 +123,7 @@ rules:
   - watch
   - create
   - patch
+  - update
   - delete
 - apiGroups:
   - serving.kserve.io
@@ -121,3 +131,15 @@ rules:
   - servingruntimes
   verbs:
   - create
+- apiGroups:
+    - nim.opendatahub.io
+  resources:
+    - accounts
+  verbs:
+    - watch
+    - update
+    - get
+    - list
+    - create
+    - patch
+    - delete