From cb1d6283391f39389f5448b025646264b01b469e Mon Sep 17 00:00:00 2001 From: Quan Pham Date: Fri, 3 Oct 2025 12:11:31 -0400 Subject: [PATCH] Allow pushing user-allocation membership to Keycloak A Keycloak admin client has been added When `activate_allocation` is called, the user is added to a Keycloak group named after the project ID on the remote cluster. If the user does not already exist in Keycloak, the case is ignored for now When `deactivate_allocation` is called, the user is removed from the Keycloak group Unit tests have been updated to remove dependancy on Keycloak A comment in `validate_allocations` has been updated to reflect the more restrictive validation behavior, where users on cluster projects will be removed if they are not part of the Coldfront allocation (rather than if they are registered on Coldfront at all). This is relevant for functional tests for this new feature. --- .../workflows/test-functional-microshift.yaml | 4 + .../workflows/test-functional-microstack.yaml | 4 + ci/run_functional_tests_openshift.sh | 6 ++ ci/run_functional_tests_openstack.sh | 6 ++ ci/setup-keycloak.sh | 10 +++ requirements.txt | 1 + src/coldfront_plugin_cloud/base.py | 32 +++++-- src/coldfront_plugin_cloud/kc_client.py | 86 +++++++++++++++++++ .../commands/validate_allocations.py | 4 +- src/coldfront_plugin_cloud/openshift.py | 4 + src/coldfront_plugin_cloud/openstack.py | 2 + src/coldfront_plugin_cloud/tests/base.py | 17 ++-- .../functional/openshift/test_allocation.py | 9 +- .../functional/openstack/test_allocation.py | 15 +++- .../tests/unit/openshift/test_rbac.py | 1 + .../unit/test_calculate_quota_unit_hours.py | 20 ++--- .../unit/test_migrate_field_of_science.py | 9 +- 17 files changed, 197 insertions(+), 33 deletions(-) create mode 100644 ci/setup-keycloak.sh create mode 100644 src/coldfront_plugin_cloud/kc_client.py diff --git a/.github/workflows/test-functional-microshift.yaml b/.github/workflows/test-functional-microshift.yaml index 110e91ac..1f0af2a4 100644 --- a/.github/workflows/test-functional-microshift.yaml +++ b/.github/workflows/test-functional-microshift.yaml @@ -35,6 +35,10 @@ jobs: run: | bash ./ci/setup-oc-client.sh + - name: Install Keycloak + run: | + bash ./ci/setup-keycloak.sh + - name: Install Microshift run: | ./ci/microshift.sh diff --git a/.github/workflows/test-functional-microstack.yaml b/.github/workflows/test-functional-microstack.yaml index 594fdf99..1ad60344 100644 --- a/.github/workflows/test-functional-microstack.yaml +++ b/.github/workflows/test-functional-microstack.yaml @@ -18,6 +18,10 @@ jobs: with: python-version: 3.12 + - name: Install Keycloak + run: | + bash ./ci/setup-keycloak.sh + - name: Install ColdFront and plugin run: | python -m pip install --upgrade pip diff --git a/ci/run_functional_tests_openshift.sh b/ci/run_functional_tests_openshift.sh index 2aa9125d..bef92194 100755 --- a/ci/run_functional_tests_openshift.sh +++ b/ci/run_functional_tests_openshift.sh @@ -5,6 +5,12 @@ # Tests expect the resource to be name Devstack set -xe +export KEYCLOAK_BASE_URL="http://localhost:8080" +export KEYCLOAK_REALM="master" +export KEYCLOAK_CLIENT_ID="admin-cli" +export KEYCLOAK_ADMIN_USER="admin" +export KEYCLOAK_ADMIN_PASSWORD="nomoresecret" + export OPENSHIFT_MICROSHIFT_TOKEN="$(oc create token -n onboarding onboarding-serviceaccount)" export OPENSHIFT_MICROSHIFT_VERIFY="false" diff --git a/ci/run_functional_tests_openstack.sh b/ci/run_functional_tests_openstack.sh index b6aa1c67..a60cd1df 100755 --- a/ci/run_functional_tests_openstack.sh +++ b/ci/run_functional_tests_openstack.sh @@ -5,6 +5,12 @@ # Tests expect the resource to be name Devstack set -xe +export KEYCLOAK_BASE_URL="http://localhost:8080" +export KEYCLOAK_REALM="master" +export KEYCLOAK_CLIENT_ID="admin-cli" +export KEYCLOAK_ADMIN_USER="admin" +export KEYCLOAK_ADMIN_PASSWORD="nomoresecret" + export CREDENTIAL_NAME=$(openssl rand -base64 12) export OPENSTACK_DEVSTACK_APPLICATION_CREDENTIAL_SECRET=$( diff --git a/ci/setup-keycloak.sh b/ci/setup-keycloak.sh new file mode 100644 index 00000000..c1524f55 --- /dev/null +++ b/ci/setup-keycloak.sh @@ -0,0 +1,10 @@ +#!/bin/bash + +set -xe + +sudo docker run -d --name keycloak \ + -e KEYCLOAK_ADMIN=admin \ + -e KEYCLOAK_ADMIN_PASSWORD=nomoresecret \ + -p 8080:8080 \ + -p 8443:8443 \ + quay.io/keycloak/keycloak:25.0 start-dev diff --git a/requirements.txt b/requirements.txt index 181a2626..a52194f3 100644 --- a/requirements.txt +++ b/requirements.txt @@ -8,3 +8,4 @@ python-keystoneclient python-novaclient python-neutronclient python-swiftclient +requests diff --git a/src/coldfront_plugin_cloud/base.py b/src/coldfront_plugin_cloud/base.py index 00a4807d..656f5399 100644 --- a/src/coldfront_plugin_cloud/base.py +++ b/src/coldfront_plugin_cloud/base.py @@ -1,11 +1,14 @@ import abc import functools from typing import NamedTuple +import logging from coldfront.core.allocation import models as allocation_models from coldfront.core.resource import models as resource_models -from coldfront_plugin_cloud import attributes +from coldfront_plugin_cloud import attributes, kc_client + +logger = logging.getLogger(__name__) class ResourceAllocator(abc.ABC): @@ -25,11 +28,30 @@ def __init__( self.resource = resource self.allocation = allocation + @functools.cached_property + def kc_admin_client(self): + return kc_client.KeyCloakAPIClient() + def get_or_create_federated_user(self, username): if not (user := self.get_federated_user(username)): user = self.create_federated_user(username) return user + def assign_role_on_user(self, username, project_id): + self.kc_admin_client.create_group(project_id) + if user_id := self.kc_admin_client.get_user_id(username): + group_id = self.kc_admin_client.get_group_id(project_id) + self.kc_admin_client.add_user_to_group(user_id, group_id) + else: + logger.warning( + f"User {username} not found in Keycloak, cannot add to group." + ) + + def remove_role_from_user(self, username, project_id): + user_id = self.kc_admin_client.get_user_id(username) + group_id = self.kc_admin_client.get_group_id(project_id) + self.kc_admin_client.remove_user_from_group(user_id, group_id) + @functools.cached_property def auth_url(self): return self.resource.get_attribute(attributes.RESOURCE_AUTH_URL).rstrip("/") @@ -69,11 +91,3 @@ def create_federated_user(self, unique_id): @abc.abstractmethod def get_federated_user(self, unique_id): pass - - @abc.abstractmethod - def assign_role_on_user(self, username, project_id): - pass - - @abc.abstractmethod - def remove_role_from_user(self, username, project_id): - pass diff --git a/src/coldfront_plugin_cloud/kc_client.py b/src/coldfront_plugin_cloud/kc_client.py new file mode 100644 index 00000000..574ad2c9 --- /dev/null +++ b/src/coldfront_plugin_cloud/kc_client.py @@ -0,0 +1,86 @@ +import os +import functools + +import requests + + +class KeyCloakAPIClient: + def __init__(self): + self.base_url = os.getenv("KEYCLOAK_BASE_URL") + self.realm = os.getenv("KEYCLOAK_REALM") + self.admin_user = os.getenv("KEYCLOAK_ADMIN_USER") + self.admin_password = os.getenv("KEYCLOAK_ADMIN_PASSWORD") + self.client_id = os.getenv("KEYCLOAK_CLIENT_ID", "admin-cli") + + self.token_url = ( + f"{self.base_url}/realms/{self.realm}/protocol/openid-connect/token" + ) + + @functools.cached_property + def api_client(self): + params = { + "grant_type": "password", + "client_id": self.client_id, + "username": self.admin_user, + "password": self.admin_password, + "scope": "openid", + } + r = requests.post(self.token_url, data=params).json() + headers = { + "Authorization": ("Bearer %s" % r["access_token"]), + "Content-Type": "application/json", + } + session = requests.session() + session.headers.update(headers) + return session + + def create_group(self, group_name): + url = f"{self.base_url}/admin/realms/{self.realm}/groups" + payload = {"name": group_name} + response = self.api_client.post(url, json=payload) + + # If group already exists, ignore and move on + if response.status_code not in (201, 409): + response.raise_for_status() + + def create_user(self, cf_username): + """Helper function to create user in Keycloak, for testing purposes only""" + url = f"{self.base_url}/admin/realms/{self.realm}/users" + payload = { + "username": cf_username, + "enabled": True, + "email": cf_username, + } + r = self.api_client.post(url, json=payload) + r.raise_for_status() + + def get_group_id(self, group_name) -> str | None: + """Return None if group not found""" + query = f"search={group_name}&exact=true" + url = f"{self.base_url}/admin/realms/{self.realm}/groups?{query}" + r = self.api_client.get(url).json() + return r[0]["id"] if r else None + + def get_user_id(self, cf_username) -> str | None: + """Return None if user not found""" + # TODO (Quan): Confirm that Coldfront usernames map to Keycloak emails, not email, or something else? + query = f"email={cf_username}&exact=true" + url = f"{self.base_url}/admin/realms/{self.realm}/users?{query}" + r = self.api_client.get(url).json() + return r[0]["id"] if r else None + + def add_user_to_group(self, user_id, group_id): + url = f"{self.base_url}/admin/realms/{self.realm}/users/{user_id}/groups/{group_id}" + r = self.api_client.put(url) + r.raise_for_status() + + def remove_user_from_group(self, user_id, group_id): + url = f"{self.base_url}/admin/realms/{self.realm}/users/{user_id}/groups/{group_id}" + r = self.api_client.delete(url) + r.raise_for_status() + + def get_user_groups(self, user_id) -> list[str]: + url = f"{self.base_url}/admin/realms/{self.realm}/users/{user_id}/groups" + r = self.api_client.get(url) + r.raise_for_status() + return [group["name"] for group in r.json()] diff --git a/src/coldfront_plugin_cloud/management/commands/validate_allocations.py b/src/coldfront_plugin_cloud/management/commands/validate_allocations.py index ef5ea1ff..7fbeb45b 100644 --- a/src/coldfront_plugin_cloud/management/commands/validate_allocations.py +++ b/src/coldfront_plugin_cloud/management/commands/validate_allocations.py @@ -50,7 +50,7 @@ def sync_users(project_id, allocation, allocator, apply): if apply: tasks.add_user_to_allocation(coldfront_user.pk) - # remove users that are in the resource but not in coldfront + # remove users that are in the resource but not in coldfront allocation users = set( [coldfront_user.user.username for coldfront_user in coldfront_users] ) @@ -58,7 +58,7 @@ def sync_users(project_id, allocation, allocator, apply): if allocation_user not in users: failed_validation = True logger.warn( - f"{allocation_user} exists in the resource {project_id} but not in coldfront" + f"{allocation_user} exists in the resource {project_id} but not in coldfront allocation" ) if apply: allocator.remove_role_from_user(allocation_user, project_id) diff --git a/src/coldfront_plugin_cloud/openshift.py b/src/coldfront_plugin_cloud/openshift.py index 91b39cdf..e2780a12 100644 --- a/src/coldfront_plugin_cloud/openshift.py +++ b/src/coldfront_plugin_cloud/openshift.py @@ -253,6 +253,8 @@ def assign_role_on_user(self, username, project_id): # Role already exists, ignore pass + super().assign_role_on_user(username, project_id) + def remove_role_from_user(self, username, project_id): """Remove a role from a user in a project using direct OpenShift API calls""" try: @@ -275,6 +277,8 @@ def remove_role_from_user(self, username, project_id): # Rolebinding doesn't exist, nothing to remove pass + super().remove_role_from_user(username, project_id) + def _create_project(self, project_name, project_id): pi_username = self.allocation.project.pi.username diff --git a/src/coldfront_plugin_cloud/openstack.py b/src/coldfront_plugin_cloud/openstack.py index b47ec84b..514a2c6c 100644 --- a/src/coldfront_plugin_cloud/openstack.py +++ b/src/coldfront_plugin_cloud/openstack.py @@ -344,12 +344,14 @@ def assign_role_on_user(self, username, project_id): user = self.get_federated_user(username) self.identity.roles.grant(user=user["id"], project=project_id, role=role) + super().assign_role_on_user(username, project_id) def remove_role_from_user(self, username, project_id): role = self.identity.roles.find(name=self.member_role_name) if user := self.get_federated_user(username): self.identity.roles.revoke(user=user["id"], project=project_id, role=role) + super().remove_role_from_user(username, project_id) def create_default_network(self, project_id): neutron = neutronclient.Client(session=get_session_for_resource(self.resource)) diff --git a/src/coldfront_plugin_cloud/tests/base.py b/src/coldfront_plugin_cloud/tests/base.py index 03d35d62..44113aee 100644 --- a/src/coldfront_plugin_cloud/tests/base.py +++ b/src/coldfront_plugin_cloud/tests/base.py @@ -24,6 +24,8 @@ from coldfront.core.field_of_science.models import FieldOfScience from django.core.management import call_command +from coldfront_plugin_cloud import kc_client + class TestBase(TestCase): def setUp(self) -> None: @@ -37,11 +39,7 @@ def setUp(self) -> None: # For testing we can validate allocations with this status AllocationStatusChoice.objects.get_or_create(name="Active (Needs Renewal)") - @staticmethod - def new_user(username=None) -> User: - username = username or f"{uuid.uuid4().hex}@example.com" - User.objects.create(username=username, email=username) - return User.objects.get(username=username) + self.kc_admin_client = kc_client.KeyCloakAPIClient() @staticmethod def new_esi_resource(name=None, auth_url=None) -> Resource: @@ -101,6 +99,15 @@ def new_openshift_resource( ) return Resource.objects.get(name=resource_name) + def new_user(self, username=None, add_to_keycloak=True) -> User: + username = username or f"{uuid.uuid4().hex}@example.com" + User.objects.create(username=username, email=username) + + if add_to_keycloak: + self.kc_admin_client.create_user(username) + + return User.objects.get(username=username) + def new_project(self, title=None, pi=None) -> Project: title = title or uuid.uuid4().hex pi = pi or self.new_user() diff --git a/src/coldfront_plugin_cloud/tests/functional/openshift/test_allocation.py b/src/coldfront_plugin_cloud/tests/functional/openshift/test_allocation.py index a4177792..3c501ad9 100644 --- a/src/coldfront_plugin_cloud/tests/functional/openshift/test_allocation.py +++ b/src/coldfront_plugin_cloud/tests/functional/openshift/test_allocation.py @@ -2,7 +2,6 @@ import time import unittest from unittest import mock -import uuid from coldfront_plugin_cloud import attributes, openshift, tasks, utils from coldfront_plugin_cloud.tests import base @@ -52,7 +51,13 @@ def test_new_allocation(self): allocator._get_role(user.username, project_id) + # Check Keycloak group and user membership + self.kc_admin_client.get_group_id(project_id) + user_id = self.kc_admin_client.get_user_id(user.username) + assert project_id in self.kc_admin_client.get_user_groups(user_id) + allocator.remove_role_from_user(user.username, project_id) + assert project_id not in self.kc_admin_client.get_user_groups(user_id) with self.assertRaises(openshift.NotFound): allocator._get_role(user.username, project_id) @@ -109,7 +114,7 @@ def test_add_remove_user(self): # directly add a user to openshift which should then be # deleted when validate_allocations is called - non_coldfront_user = uuid.uuid4().hex + non_coldfront_user = self.new_user(add_to_keycloak=True).username allocator.get_or_create_federated_user(non_coldfront_user) allocator.assign_role_on_user(non_coldfront_user, project_id) assert non_coldfront_user in allocator.get_users(project_id) diff --git a/src/coldfront_plugin_cloud/tests/functional/openstack/test_allocation.py b/src/coldfront_plugin_cloud/tests/functional/openstack/test_allocation.py index 20a24a61..6b83c7f8 100644 --- a/src/coldfront_plugin_cloud/tests/functional/openstack/test_allocation.py +++ b/src/coldfront_plugin_cloud/tests/functional/openstack/test_allocation.py @@ -1,6 +1,5 @@ import os import unittest -import uuid import time from coldfront_plugin_cloud import attributes, openstack, tasks, utils @@ -57,6 +56,11 @@ def test_new_allocation(self): self.assertEqual(len(roles), 1) self.assertEqual(roles[0].role["id"], self.role_member.id) + # Check Keycloak group and user membership + self.kc_admin_client.get_group_id(project_id) + user_id = self.kc_admin_client.get_user_id(user.username) + assert project_id in self.kc_admin_client.get_user_groups(user_id) + # Check default network # Port build-up time is not instant time.sleep(5) @@ -297,6 +301,11 @@ def test_add_remove_user(self): tasks.add_user_to_allocation(allocation_user2.pk) + # Check Keycloak group and user membership + self.kc_admin_client.get_group_id(project_id) + user2_id = self.kc_admin_client.get_user_id(user2.username) + assert project_id in self.kc_admin_client.get_user_groups(user2_id) + openstack_user = allocator.get_federated_user(user2.username) openstack_user = self.identity.users.get(openstack_user["id"]) @@ -310,6 +319,8 @@ def test_add_remove_user(self): tasks.remove_user_from_allocation(allocation_user2.pk) + assert project_id not in self.kc_admin_client.get_user_groups(user2_id) + roles = self.identity.role_assignments.list( user=openstack_user.id, project=openstack_project.id ) @@ -326,7 +337,7 @@ def test_add_remove_user(self): # directly add a user to openstack which should then be # deleted when validate_allocations is called - non_coldfront_user = uuid.uuid4().hex + non_coldfront_user = self.new_user(add_to_keycloak=True).username allocator.get_or_create_federated_user(non_coldfront_user) allocator.assign_role_on_user(non_coldfront_user, project_id) assert non_coldfront_user in allocator.get_users(project_id) diff --git a/src/coldfront_plugin_cloud/tests/unit/openshift/test_rbac.py b/src/coldfront_plugin_cloud/tests/unit/openshift/test_rbac.py index b7898f7c..e29c91b7 100644 --- a/src/coldfront_plugin_cloud/tests/unit/openshift/test_rbac.py +++ b/src/coldfront_plugin_cloud/tests/unit/openshift/test_rbac.py @@ -13,6 +13,7 @@ def setUp(self) -> None: self.allocator = OpenShiftResourceAllocator(mock_resource, mock_allocation) self.allocator.id_provider = "fake_idp" self.allocator.k8_client = mock.Mock() + self.allocator.kc_admin_client = mock.Mock() self.allocator.member_role_name = "admin" def test_user_in_rolebindings_false(self): diff --git a/src/coldfront_plugin_cloud/tests/unit/test_calculate_quota_unit_hours.py b/src/coldfront_plugin_cloud/tests/unit/test_calculate_quota_unit_hours.py index f704cdef..eba3f329 100644 --- a/src/coldfront_plugin_cloud/tests/unit/test_calculate_quota_unit_hours.py +++ b/src/coldfront_plugin_cloud/tests/unit/test_calculate_quota_unit_hours.py @@ -22,7 +22,7 @@ def test_new_allocation_quota(self): ) with freezegun.freeze_time("2020-03-15 00:01:00"): - user = self.new_user() + user = self.new_user(add_to_keycloak=False) project = self.new_project(pi=user) allocation = self.new_allocation(project, self.resource, 2) utils.set_attribute_on_allocation( @@ -92,7 +92,7 @@ def test_new_allocation_quota_expired(self): self.resource = self.new_openshift_resource( name="", ) - user = self.new_user() + user = self.new_user(add_to_keycloak=False) project = self.new_project(pi=user) allocation = self.new_allocation(project, self.resource, 2) allocation.status = allocation_models.AllocationStatusChoice.objects.get( @@ -125,7 +125,7 @@ def test_new_allocation_quota_denied(self): self.resource = self.new_openshift_resource( name="", ) - user = self.new_user() + user = self.new_user(add_to_keycloak=False) project = self.new_project(pi=user) allocation = self.new_allocation(project, self.resource, 2) @@ -155,7 +155,7 @@ def test_new_allocation_quota_last_revoked(self): self.resource = self.new_openshift_resource( name="", ) - user = self.new_user() + user = self.new_user(add_to_keycloak=False) project = self.new_project(pi=user) allocation = self.new_allocation(project, self.resource, 2) @@ -200,7 +200,7 @@ def test_new_allocation_quota_new(self): self.resource = self.new_openshift_resource( name="", ) - user = self.new_user() + user = self.new_user(add_to_keycloak=False) project = self.new_project(pi=user) allocation = self.new_allocation(project, self.resource, 2) @@ -218,7 +218,7 @@ def test_new_allocation_quota_never_approved(self): self.resource = self.new_openshift_resource( name="", ) - user = self.new_user() + user = self.new_user(add_to_keycloak=False) project = self.new_project(pi=user) allocation = self.new_allocation(project, self.resource, 2) @@ -240,7 +240,7 @@ def test_change_request_decrease(self): self.resource = self.new_openshift_resource( name="", ) - user = self.new_user() + user = self.new_user(add_to_keycloak=False) project = self.new_project(pi=user) allocation = self.new_allocation(project, self.resource, 2) @@ -286,7 +286,7 @@ def test_change_request_increase(self): self.resource = self.new_openshift_resource( name="", ) - user = self.new_user() + user = self.new_user(add_to_keycloak=False) project = self.new_project(pi=user) allocation = self.new_allocation(project, self.resource, 2) @@ -332,7 +332,7 @@ def test_change_request_decrease_multiple(self): self.resource = self.new_openshift_resource( name="", ) - user = self.new_user() + user = self.new_user(add_to_keycloak=False) project = self.new_project(pi=user) allocation = self.new_allocation(project, self.resource, 2) @@ -395,7 +395,7 @@ def test_new_allocation_quota_change_request(self): self.resource = self.new_openshift_resource( name="", ) - user = self.new_user() + user = self.new_user(add_to_keycloak=False) project = self.new_project(pi=user) allocation = self.new_allocation(project, self.resource, 2) diff --git a/src/coldfront_plugin_cloud/tests/unit/test_migrate_field_of_science.py b/src/coldfront_plugin_cloud/tests/unit/test_migrate_field_of_science.py index 8bc11f1a..56f5b0e4 100644 --- a/src/coldfront_plugin_cloud/tests/unit/test_migrate_field_of_science.py +++ b/src/coldfront_plugin_cloud/tests/unit/test_migrate_field_of_science.py @@ -18,9 +18,12 @@ def test_command_output(self): new_fos_1_des = uuid.uuid4().hex # Migrate to new fos new_fos_2_des = old_fos_4.description # Migrate to existing fos - fake_project_1 = self.new_project() - fake_project_2 = self.new_project() - fake_project_3 = self.new_project() + fake_user = self.new_user( + add_to_keycloak=False + ) # To avoid Keycloak dependency in unit test + fake_project_1 = self.new_project(pi=fake_user) + fake_project_2 = self.new_project(pi=fake_user) + fake_project_3 = self.new_project(pi=fake_user) fake_project_1.field_of_science = old_fos_1 fake_project_2.field_of_science = old_fos_2 fake_project_3.field_of_science = old_fos_3