Allow using validate_allocations to validate Keycloak group memberships#307
Allow using validate_allocations to validate Keycloak group memberships#307QuanMPhm wants to merge 1 commit into
validate_allocations to validate Keycloak group memberships#307Conversation
QuanMPhm
force-pushed
the
ops_948/kc_validate
branch
from
f874adf to
68364bb
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the validate_allocations management command to reconcile ColdFront allocation membership with Keycloak group membership (add missing users and remove stale users) when Keycloak integration is enabled, and adjusts the Keycloak functional tests to validate this behavior through the command rather than directly calling task functions.
Changes:
- Add Keycloak group membership reconciliation logic to
validate_allocations(conditional on Keycloak being enabled). - Extend the Keycloak API client with a
get_group_members()helper to support membership validation. - Update functional Keycloak tests to call
validate_allocations --applyand patch allocator discovery to avoid remote calls.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/coldfront_plugin_cloud/tests/functional/keycloak/test_keycloak.py |
Updates tests to validate Keycloak membership changes via validate_allocations --apply instead of direct task calls. |
src/coldfront_plugin_cloud/management/commands/validate_allocations.py |
Adds Keycloak group membership reconciliation during allocation validation. |
src/coldfront_plugin_cloud/kc_client.py |
Adds Keycloak API helper to list group members used by reconciliation logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def validate_keycloak_group_memberships(self, allocation: Allocation, apply: bool): | ||
| kc_client = tasks.get_kc_client() | ||
| resource = allocation.resources.first() |
| allocation_users: list[AllocationUser] = allocation.allocationuser_set.all() | ||
| allocation_usernames = set(au.user.username for au in allocation_users) | ||
|
|
There was a problem hiding this comment.
From local testing, iterating over 100 test allocations and getting the allocation.project.title, the time improvement is 10x. Seems this will be a good idea, given that some allocations have +50 users for large classes.
| group_name = tasks._get_keycloak_group_name(allocation, group_template) | ||
| group_id = kc_client.get_group_id(group_name) |
| user_id = kc_client.get_user_id(username) | ||
| kc_client.remove_user_from_group(user_id, group_id) |
| def get_group_members(self, group_id) -> list[str]: | ||
| url = f"{self.base_url}/admin/realms/{self.realm}/groups/{group_id}/members" | ||
| r = self.api_client.get(url) | ||
| r.raise_for_status() | ||
| users = UserResponse.model_validate(r.json()) | ||
| return [user.username for user in users.root] |
There was a problem hiding this comment.
Implemented pagination, and added a functional test to test pagination functionality
| for au in to_add: | ||
| logger.info( | ||
| f"Adding user {au.user.username} to Keycloak group {group_name}" | ||
| ) | ||
| if apply: | ||
| tasks.add_user_to_keycloak(au.pk) | ||
| for username in to_remove: |
`validate_allocations` command will now add users to Keycloak groups if they are part of an allocation, and remove them if they are not. Updated keyclaok functional tests
QuanMPhm
force-pushed
the
ops_948/kc_validate
branch
from
68364bb to
8f5eb06
Compare
knikolla
left a comment
knikolla
left a comment
There was a problem hiding this comment.
Structural feedback. This is reintroducing a lot of code repetition that we've just worked to make reusable. There's ways and opportunities to plug into that and we should investigate and pursue those.
| ) | ||
| continue | ||
|
|
||
| if signals.is_keycloak_enabled(): |
There was a problem hiding this comment.
I don't like that we're importing signals for this, the function is_keycloak_enabled() should be moved. What do you think would be a more appropriate place?
|
|
||
| def validate_keycloak_group_memberships(self, allocation: Allocation, apply: bool): | ||
| # Fetch or cache Keycloak client | ||
| kc_client = getattr(self, "_kc_client", None) |
There was a problem hiding this comment.
This is basically what a cached_property does in an easier to read way.
| ) | ||
| return | ||
|
|
||
| allocation_users: list[AllocationUser] = ( |
There was a problem hiding this comment.
We just found a way to make this part of the code reusable in
Can you try to think of a way to plug this into that or something similar?
Closes #306. Part of nerc-project/operations#948. Dependant on #249. This PR consists of the last commit