Adding Admin Checks to Node Operations. (#1779)

* Adding Admin Checks to Node Operations.

* Importing function.

* Changing naming convention.

* Fixing webhook events.

* Adding changes to scaleset init.

docs/webhook_events.md

@@ -681,7 +681,6 @@ If webhook is set to have Event Grid message format then the payload will look a
         "admins": [
             "00000000-0000-0000-0000-000000000000"
         ],
-        "allow_pool_management": true,
         "allowed_aad_tenants": [
             "00000000-0000-0000-0000-000000000000"
         ],
@@ -693,7 +692,8 @@ If webhook is set to have Event Grid message format then the payload will look a
             "allowed_ips": [],
             "allowed_service_tags": []
         },
-        "proxy_vm_sku": "Standard_B2s"
+        "proxy_vm_sku": "Standard_B2s",
+        "require_admin_privileges": true
     }
 }
 ```
@@ -809,11 +809,6 @@ If webhook is set to have Event Grid message format then the payload will look a
                     "title": "Admins",
                     "type": "array"
                 },
-                "allow_pool_management": {
-                    "default": true,
-                    "title": "Allow Pool Management",
-                    "type": "boolean"
-                },
                 "allowed_aad_tenants": {
                     "items": {
                         "format": "uuid",
@@ -854,6 +849,11 @@ If webhook is set to have Event Grid message format then the payload will look a
                     "title": "Proxy Vm Sku",
                     "type": "string"
                 },
+                "require_admin_privileges": {
+                    "default": true,
+                    "title": "Require Admin Privileges",
+                    "type": "boolean"
+                },
                 "vm_tags": {
                     "additionalProperties": {
                         "type": "string"
@@ -6006,11 +6006,6 @@ If webhook is set to have Event Grid message format then the payload will look a
                     "title": "Admins",
                     "type": "array"
                 },
-                "allow_pool_management": {
-                    "default": true,
-                    "title": "Allow Pool Management",
-                    "type": "boolean"
-                },
                 "allowed_aad_tenants": {
                     "items": {
                         "format": "uuid",
@@ -6051,6 +6046,11 @@ If webhook is set to have Event Grid message format then the payload will look a
                     "title": "Proxy Vm Sku",
                     "type": "string"
                 },
+                "require_admin_privileges": {
+                    "default": true,
+                    "title": "Require Admin Privileges",
+                    "type": "boolean"
+                },
                 "vm_tags": {
                     "additionalProperties": {
                         "type": "string"

src/api-service/__app__/node/__init__.py

@@ -9,7 +9,7 @@ from onefuzztypes.models import Error
 from onefuzztypes.requests import NodeGet, NodeSearch, NodeUpdate
 from onefuzztypes.responses import BoolResult
 
-from ..onefuzzlib.endpoint_authorization import call_if_user
+from ..onefuzzlib.endpoint_authorization import call_if_user, check_require_admins
 from ..onefuzzlib.request import not_ok, ok, parse_request
 from ..onefuzzlib.workers.nodes import Node, NodeMessage, NodeTasks
 
@@ -50,6 +50,10 @@ def post(req: func.HttpRequest) -> func.HttpResponse:
     if isinstance(request, Error):
         return not_ok(request, context="NodeUpdate")
 
+    answer = check_require_admins(req)
+    if isinstance(answer, Error):
+        return not_ok(answer, context="NodeUpdate")
+
     node = Node.get_by_machine_id(request.machine_id)
     if not node:
         return not_ok(
@@ -68,6 +72,10 @@ def delete(req: func.HttpRequest) -> func.HttpResponse:
     if isinstance(request, Error):
         return not_ok(request, context="NodeDelete")
 
+    answer = check_require_admins(req)
+    if isinstance(answer, Error):
+        return not_ok(answer, context="NodeDelete")
+
     node = Node.get_by_machine_id(request.machine_id)
     if not node:
         return not_ok(
@@ -88,6 +96,10 @@ def patch(req: func.HttpRequest) -> func.HttpResponse:
     if isinstance(request, Error):
         return not_ok(request, context="NodeReimage")
 
+    answer = check_require_admins(req)
+    if isinstance(answer, Error):
+        return not_ok(answer, context="NodeReimage")
+
     node = Node.get_by_machine_id(request.machine_id)
     if not node:
         return not_ok(

src/api-service/__app__/onefuzzlib/endpoint_authorization.py

@@ -110,10 +110,10 @@ def can_modify_config(req: func.HttpRequest, config: InstanceConfig) -> bool:
     return can_modify_config_impl(config, user_info)
 
 
-def check_can_manage_pools_impl(
+def check_require_admins_impl(
     config: InstanceConfig, user_info: UserInfo
 ) -> Optional[Error]:
-    if config.allow_pool_management:
+    if config.require_admin_privileges:
         return None
 
     if config.admins is None:
@@ -125,25 +125,25 @@ def check_can_manage_pools_impl(
     return Error(code=ErrorCode.UNAUTHORIZED, errors=["not authorized to manage pools"])
 
 
-def check_can_manage_pools(req: func.HttpRequest) -> Optional[Error]:
+def check_require_admins(req: func.HttpRequest) -> Optional[Error]:
     user_info = parse_jwt_token(req)
     if isinstance(user_info, Error):
         return user_info
 
     # When there are no admins in the `admins` list, all users are considered
-    # admins.  However, `allow_pool_management` is still useful to protect from
+    # admins.  However, `require_admin_privileges` is still useful to protect from
     # mistakes.
     #
     # To make changes while still protecting against accidental changes to
     # pools, do the following:
     #
-    # 1. set `allow_pool_management` to `True`
+    # 1. set `require_admin_privileges` to `True`
     # 2. make the change
-    # 3. set `allow_pool_management` to `False`
+    # 3. set `require_admin_privileges` to `False`
 
     config = InstanceConfig.fetch()
 
-    return check_can_manage_pools_impl(config, user_info)
+    return check_require_admins_impl(config, user_info)
 
 
 def is_user(token_data: UserInfo) -> bool:

src/api-service/__app__/pool/__init__.py

@@ -21,7 +21,7 @@ from ..onefuzzlib.azure.creds import (
 from ..onefuzzlib.azure.queue import get_queue_sas
 from ..onefuzzlib.azure.storage import StorageType
 from ..onefuzzlib.azure.vmss import list_available_skus
-from ..onefuzzlib.endpoint_authorization import call_if_user, check_can_manage_pools
+from ..onefuzzlib.endpoint_authorization import call_if_user, check_require_admins
 from ..onefuzzlib.request import not_ok, ok, parse_request
 from ..onefuzzlib.workers.pools import Pool
 
@@ -77,7 +77,7 @@ def post(req: func.HttpRequest) -> func.HttpResponse:
     if isinstance(request, Error):
         return not_ok(request, context="PoolCreate")
 
-    answer = check_can_manage_pools(req)
+    answer = check_require_admins(req)
     if isinstance(answer, Error):
         return not_ok(answer, context="PoolCreate")
 
@@ -133,7 +133,7 @@ def delete(req: func.HttpRequest) -> func.HttpResponse:
     if isinstance(request, Error):
         return not_ok(request, context="PoolDelete")
 
-    answer = check_can_manage_pools(req)
+    answer = check_require_admins(req)
     if isinstance(answer, Error):
         return not_ok(answer, context="PoolDelete")
 

src/api-service/__app__/scaleset/__init__.py

@@ -17,7 +17,7 @@ from onefuzztypes.responses import BoolResult
 from ..onefuzzlib.azure.creds import get_base_region, get_regions
 from ..onefuzzlib.azure.vmss import list_available_skus
 from ..onefuzzlib.config import InstanceConfig
-from ..onefuzzlib.endpoint_authorization import call_if_user, check_can_manage_pools
+from ..onefuzzlib.endpoint_authorization import call_if_user, check_require_admins
 from ..onefuzzlib.request import not_ok, ok, parse_request
 from ..onefuzzlib.workers.pools import Pool
 from ..onefuzzlib.workers.scalesets import AutoScale, Scaleset
@@ -49,7 +49,7 @@ def post(req: func.HttpRequest) -> func.HttpResponse:
     if isinstance(request, Error):
         return not_ok(request, context="ScalesetCreate")
 
-    answer = check_can_manage_pools(req)
+    answer = check_require_admins(req)
     if isinstance(answer, Error):
         return not_ok(answer, context="ScalesetCreate")
 
@@ -127,7 +127,7 @@ def delete(req: func.HttpRequest) -> func.HttpResponse:
     if isinstance(request, Error):
         return not_ok(request, context="ScalesetDelete")
 
-    answer = check_can_manage_pools(req)
+    answer = check_require_admins(req)
     if isinstance(answer, Error):
         return not_ok(answer, context="ScalesetDelete")
 
@@ -144,7 +144,7 @@ def patch(req: func.HttpRequest) -> func.HttpResponse:
     if isinstance(request, Error):
         return not_ok(request, context="ScalesetUpdate")
 
-    answer = check_can_manage_pools(req)
+    answer = check_require_admins(req)
     if isinstance(answer, Error):
         return not_ok(answer, context="ScalesetUpdate")
 

src/api-service/tests/test_auth_check.py

@@ -12,7 +12,7 @@ from onefuzztypes.models import UserInfo
 from __app__.onefuzzlib.config import InstanceConfig
 from __app__.onefuzzlib.endpoint_authorization import (
     can_modify_config_impl,
-    check_can_manage_pools_impl,
+    check_require_admins_impl,
 )
 
 if "ONEFUZZ_INSTANCE_NAME" not in os.environ:
@@ -69,9 +69,9 @@ class TestAdmin(unittest.TestCase):
 
         # by default, any can modify
         self.assertIsNone(
-            check_can_manage_pools_impl(
+            check_require_admins_impl(
                 InstanceConfig(
-                    allowed_aad_tenants=[UUID(int=0)], allow_pool_management=True
+                    allowed_aad_tenants=[UUID(int=0)], require_admin_privileges=True
                 ),
                 UserInfo(),
             )
@@ -79,9 +79,9 @@ class TestAdmin(unittest.TestCase):
 
         # with oid, but no admin
         self.assertIsNone(
-            check_can_manage_pools_impl(
+            check_require_admins_impl(
                 InstanceConfig(
-                    allowed_aad_tenants=[UUID(int=0)], allow_pool_management=True
+                    allowed_aad_tenants=[UUID(int=0)], require_admin_privileges=True
                 ),
                 UserInfo(object_id=user1),
             )
@@ -89,10 +89,10 @@ class TestAdmin(unittest.TestCase):
 
         # is admin
         self.assertIsNone(
-            check_can_manage_pools_impl(
+            check_require_admins_impl(
                 InstanceConfig(
                     allowed_aad_tenants=[UUID(int=0)],
-                    allow_pool_management=False,
+                    require_admin_privileges=False,
                     admins=[user1],
                 ),
                 UserInfo(object_id=user1),
@@ -101,10 +101,10 @@ class TestAdmin(unittest.TestCase):
 
         # no user oid set
         self.assertIsNotNone(
-            check_can_manage_pools_impl(
+            check_require_admins_impl(
                 InstanceConfig(
                     allowed_aad_tenants=[UUID(int=0)],
-                    allow_pool_management=False,
+                    require_admin_privileges=False,
                     admins=[user1],
                 ),
                 UserInfo(),
@@ -113,10 +113,10 @@ class TestAdmin(unittest.TestCase):
 
         # not an admin
         self.assertIsNotNone(
-            check_can_manage_pools_impl(
+            check_require_admins_impl(
                 InstanceConfig(
                     allowed_aad_tenants=[UUID(int=0)],
-                    allow_pool_management=False,
+                    require_admin_privileges=False,
                     admins=[user1],
                 ),
                 UserInfo(object_id=user2),

src/pytypes/onefuzztypes/models.py

@@ -872,7 +872,7 @@ class InstanceConfig(BaseModel):
     admins: Optional[List[UUID]] = None
 
     # if set, only admins can manage pools or scalesets
-    allow_pool_management: bool = Field(default=True)
+    require_admin_privileges: bool = Field(default=True)
 
     allowed_aad_tenants: List[UUID]
     network_config: NetworkConfig = Field(default_factory=NetworkConfig)