-
Notifications
You must be signed in to change notification settings - Fork 14
Allow pushing user-allocation membership to Keycloak #249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| name: test-functional-keycloak | ||
|
|
||
| on: | ||
| push: | ||
| branches: [ main ] | ||
| pull_request: | ||
| branches: [ main ] | ||
|
|
||
| jobs: | ||
| build: | ||
| runs-on: ubuntu-22.04 | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v6 | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@v6 | ||
| with: | ||
| python-version: 3.12 | ||
|
|
||
| - name: Upgrade and install packages | ||
| run: | | ||
| bash ./ci/setup-ubuntu.sh | ||
|
|
||
| - name: Install Keycloak | ||
| run: | | ||
| bash ./ci/setup-keycloak.sh | ||
|
|
||
| - name: Install ColdFront and plugin | ||
| run: | | ||
| ./ci/setup.sh | ||
|
|
||
| - name: Run functional tests | ||
| run: | | ||
| ./ci/run_functional_tests_keycloak.sh |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| #!/bin/bash | ||
| set -xe | ||
|
|
||
| if [[ ! "${CI}" == "true" ]]; then | ||
| source /tmp/coldfront_venv/bin/activate | ||
| fi | ||
|
|
||
| export DJANGO_SETTINGS_MODULE="local_settings" | ||
| export PYTHONWARNINGS="ignore:Unverified HTTPS request" | ||
|
|
||
| export KEYCLOAK_BASE_URL="http://localhost:8080" | ||
| export KEYCLOAK_REALM="master" | ||
| export KEYCLOAK_CLIENT_ID="coldfront" | ||
| export KEYCLOAK_CLIENT_SECRET="nomoresecret" | ||
|
|
||
| coverage run --source="." -m django test coldfront_plugin_cloud.tests.functional.keycloak | ||
| coverage report |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| #!/bin/bash | ||
|
|
||
| set -xe | ||
|
|
||
| sudo docker rm -f keycloak | ||
|
|
||
| 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 | ||
|
|
||
| # wait for keycloak to be ready | ||
| until curl -fsS http://localhost:8080/realms/master; do | ||
| echo "Waiting for Keycloak to be ready..." | ||
| sleep 5 | ||
| done | ||
|
|
||
| # Create client and add admin role to client's service account | ||
| ACCESS_TOKEN=$(curl -X POST "http://localhost:8080/realms/master/protocol/openid-connect/token" \ | ||
| -d "username=admin" \ | ||
| -d "password=nomoresecret" \ | ||
| -d "grant_type=password" \ | ||
| -d "client_id=admin-cli" \ | ||
| -d "scope=openid" \ | ||
| | jq -r '.access_token') | ||
|
|
||
|
|
||
| curl -X POST "http://localhost:8080/admin/realms/master/clients" \ | ||
| -H "Authorization: Bearer $ACCESS_TOKEN" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{ | ||
| "clientId": "coldfront", | ||
| "secret": "nomoresecret", | ||
| "redirectUris": ["http://localhost:8080/*"], | ||
| "serviceAccountsEnabled": true | ||
| }' | ||
|
|
||
| COLDFRONT_CLIENT_ID=$(curl -X GET "http://localhost:8080/admin/realms/master/clients?clientId=coldfront" \ | ||
| -H "Authorization: Bearer $ACCESS_TOKEN" | jq -r '.[0].id') | ||
|
|
||
|
|
||
| COLDFRONT_SERVICE_ACCOUNT_ID=$(curl -X GET "http://localhost:8080/admin/realms/master/clients/$COLDFRONT_CLIENT_ID/service-account-user" \ | ||
| -H "Authorization: Bearer $ACCESS_TOKEN" \ | ||
| -H "Content-Type: application/json" \ | ||
| | jq -r '.id') | ||
|
|
||
| ADMIN_ROLE_ID=$(curl -X GET "http://localhost:8080/admin/realms/master/roles/admin" \ | ||
| -H "Authorization: Bearer $ACCESS_TOKEN" | jq -r '.id') | ||
|
|
||
| # Add admin role to the service account user | ||
| curl -X POST "http://localhost:8080/admin/realms/master/users/$COLDFRONT_SERVICE_ACCOUNT_ID/role-mappings/realm" \ | ||
| -H "Authorization: Bearer $ACCESS_TOKEN" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '[ | ||
| { | ||
| "id": "'$ADMIN_ROLE_ID'", | ||
| "name": "admin" | ||
| } | ||
| ]' | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,3 +12,4 @@ python-novaclient | |
| python-neutronclient | ||
| python-swiftclient | ||
| pytz | ||
| requests | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| import os | ||
| import functools | ||
|
|
||
| import requests | ||
| from pydantic import BaseModel, ConfigDict, RootModel | ||
|
|
||
|
|
||
| class KeyCloakGroup(BaseModel): | ||
| """Keycloak group response model""" | ||
|
|
||
| model_config = ConfigDict(extra="allow") | ||
| id: str | ||
| name: str | ||
|
|
||
|
|
||
| class GroupResponse(RootModel): | ||
| """Wrapper for group list responses""" | ||
|
|
||
| root: list[KeyCloakGroup] | ||
|
|
||
|
|
||
| class KeyCloakUser(BaseModel): | ||
| """Keycloak user response model""" | ||
|
|
||
| model_config = ConfigDict(extra="allow") | ||
| id: str | ||
| username: str | ||
|
|
||
|
|
||
| class UserResponse(RootModel): | ||
| """Wrapper for user list responses""" | ||
|
|
||
| root: list[KeyCloakUser] | ||
|
|
||
|
|
||
| class KeyCloakAPIClient: | ||
| def __init__(self): | ||
| self.base_url = os.getenv("KEYCLOAK_BASE_URL") | ||
| self.realm = os.getenv("KEYCLOAK_REALM") | ||
| self.client_id = os.getenv("KEYCLOAK_CLIENT_ID") | ||
| self.client_secret = os.getenv("KEYCLOAK_CLIENT_SECRET") | ||
|
|
||
| self.token_url = ( | ||
| f"{self.base_url}/realms/{self.realm}/protocol/openid-connect/token" | ||
| ) | ||
|
|
||
| @functools.cached_property | ||
| def api_client(self): | ||
| params = { | ||
| "grant_type": "client_credentials", | ||
| "client_id": self.client_id, | ||
| "client_secret": self.client_secret, | ||
| } | ||
| r = requests.post(self.token_url, data=params) | ||
| r.raise_for_status() | ||
| headers = { | ||
| "Authorization": ("Bearer %s" % r.json()["access_token"]), | ||
| "Content-Type": "application/json", | ||
| } | ||
| session = requests.session() | ||
| session.headers.update(headers) | ||
| return session | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are creating and caching this session with a token (and cache the instantiation of the class again later). When does the keycloak token expire? Just wondering how long we can continue to use this. Or is that not a concern since we'll run this thing as needed and then when its needed we start from scratch . I admit I don't know how coldfront tasks call these.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @naved001 The keycloak API will be called whenever Coldfront's @naved001 @knikolla Do we still want to cache the api client? Or make a follow-up PR to set a TTL mechanism?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just remove the caching for now. |
||
|
|
||
| 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) | ||
|
naved001 marked this conversation as resolved.
|
||
|
|
||
| # If group already exists, ignore and move on | ||
| if response.status_code not in (201, 409): | ||
| response.raise_for_status() | ||
|
|
||
| def get_group_id(self, group_name) -> str | None: | ||
| """Return None if group not found""" | ||
| query = { | ||
| "search": group_name, | ||
| "exact": "true", | ||
| } | ||
| url = f"{self.base_url}/admin/realms/{self.realm}/groups" | ||
| r = self.api_client.get(url, params=query) | ||
| r.raise_for_status() | ||
| groups = GroupResponse.model_validate(r.json()) | ||
| return groups.root[0].id if groups.root else None | ||
|
|
||
| def get_user_id(self, cf_username) -> str | None: | ||
| """Return None if user not found""" | ||
| # (Quan) Coldfront usernames map to Keycloak usernames | ||
| # https://github.com/nerc-project/coldfront-plugin-cloud/pull/249#discussion_r2953393852 | ||
| query = {"username": cf_username, "exact": "true"} | ||
| url = f"{self.base_url}/admin/realms/{self.realm}/users" | ||
| r = self.api_client.get(url, params=query) | ||
| r.raise_for_status() | ||
| users = UserResponse.model_validate(r.json()) | ||
| return users.root[0].id if users.root 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() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the user is already part of the group don't error out here, just log an info.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From local testing, if the user is already part of the group, 204 is returned, so not error is raised. |
||
|
|
||
| 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() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the user was not part of the group, don't error out here, just log an info.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From local testing, it seems if the user is already not part of the group, |
||
|
|
||
| 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() | ||
| groups = GroupResponse.model_validate(r.json()) | ||
| return [group.name for group in groups.root] | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,8 +6,10 @@ | |||||||||||||||||
| from coldfront_plugin_cloud.tasks import ( | ||||||||||||||||||
| activate_allocation, | ||||||||||||||||||
| add_user_to_allocation, | ||||||||||||||||||
| add_user_to_keycloak, | ||||||||||||||||||
| disable_allocation, | ||||||||||||||||||
| remove_user_from_allocation, | ||||||||||||||||||
| remove_user_from_keycloak, | ||||||||||||||||||
| ) | ||||||||||||||||||
| from coldfront.core.allocation.signals import ( | ||||||||||||||||||
| allocation_activate, | ||||||||||||||||||
|
|
@@ -25,6 +27,10 @@ def is_async(): | |||||||||||||||||
| return os.getenv("REDIS_HOST") | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| def is_keycloak_enabled(): | ||||||||||||||||||
| return os.getenv("KEYCLOAK_BASE_URL") | ||||||||||||||||||
|
||||||||||||||||||
| return os.getenv("KEYCLOAK_BASE_URL") | |
| required_env_vars = ( | |
| "KEYCLOAK_BASE_URL", | |
| "KEYCLOAK_REALM", | |
| "KEYCLOAK_CLIENT_ID", | |
| "KEYCLOAK_CLIENT_SECRET", | |
| ) | |
| return all(os.getenv(env_var) for env_var in required_env_vars) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think one env var is enough to indicate Keycloak is enabled. It's the operator's problem if the config is misconfigured
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If all of those variables are required we should be validating that they are set and emitting a useful error message if not. On the other hand, this might be the wrong place for it.
Maybe in KeyCloakAPIClient, where you read in the variables to set up the client:
class KeyCloakAPIClient:
def __init__(self):
self.base_url = os.getenv("KEYCLOAK_BASE_URL")
self.realm = os.getenv("KEYCLOAK_REALM")
self.client_id = os.getenv("KEYCLOAK_CLIENT_ID")
self.client_secret = os.getenv("KEYCLOAK_CLIENT_SECRET")...but have you considered pydantic-settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@knikolla Are you fine adding a pydantic settings module in this PR, or hold this off to a PR that puts ALL the settings in the repo into a settings model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With
set -e,docker rm -f keycloakwill cause the script to exit if the container doesn't exist (common on first run). Add|| true(or similar) so setup remains idempotent.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QuanMPhm why the thumbs down? This seems like a valid critique. A shell script with
set -eactive will exit if any command fails with an error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@larsks From local testing, running
docker rmwith the-foption makes the command return 0 if the container doesn't exist.When I saw this comment, I remember we also have a similar statement in one of the other CI files, and quickly tested the command behavior.