Moira lists to Keycloak groups #1324

marslanabdulrauf · 2025-08-04T14:48:18Z

What are the relevant tickets?

https://github.com/mitodl/hq/issues/6821#issuecomment-3149755740

Description (What does it do?)

This PR:

Migrate Moira lists usage with Keycloak groups
Fix/update Management command to migrate moira lists to keycloak

How can this be tested?

Checkout to this branch and run docker compose up --build -d
See if you have any moira lists or you can add one for testing
Run the management command to migrate all users from that moira list to keycloak with same name group python manage.py migrate_moira_to_keycloak
Go to Keycloak and verify if the users are created or not under the same group
Add/Update moira list from frontend in the collection and see if that works fine.
Try getting the group info from APIs
- http://ovs.odl.local:8089/api/v0/moira/list/repalce-me-with-group-name
- http://ovs.odl.local:8089/api/v0/groups/user/replace-me-with-user-email
Transcoding a video should also send email to users added in the collection admin_lists
Make sure the list contains all the users from that group/list

gemini-code-assist

Summary of Changes

Hello @marslanabdulrauf, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request undertakes a significant architectural shift by migrating the application's user group and access management from the legacy Moira lists system to Keycloak groups. This involves a comprehensive update across various components, including API endpoints, internal utility functions, email notification logic, and the Keycloak realm configuration itself, to fully leverage Keycloak's identity and access management capabilities. The changes aim to modernize the system's group management infrastructure and ensure seamless operation with Keycloak.

Highlights

Migration to Keycloak Groups: The core change is the complete transition from using Moira lists for user group management to Keycloak groups. This involves updating API endpoints, internal utility functions, and data models to interact with Keycloak.
Keycloak Configuration Updates: The pull request includes significant updates to the Keycloak realm configuration, such as adjusting client settings (fullScopeAllowed, bearerOnly, frontchannelLogout), adding redirectUris and webOrigins, and defining protocolMappers to correctly map user attributes (username, email, name) from Keycloak to application claims.
Refactored Keycloak Utility: A new keycloak_utils.py module centralizes Keycloak API interactions, providing methods for group and user management (e.g., creating groups, adding users to groups, listing user groups, getting group members). It also includes a helper to instantiate the Keycloak client using Django settings.
Updated Email Notification Logic: The system for sending email notifications for video events has been updated to retrieve recipient lists from Keycloak groups instead of Moira lists, ensuring that notifications are sent to the correct users based on the new group structure.
Migration Command Fixes: The management command migrate_moira_to_keycloak has been updated to correctly process all members of a Moira list during migration, fixing a previous limitation where it only processed a limited subset of members.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This PR migrates the application from using Moira lists to Keycloak groups for authorization. The changes are extensive, touching configuration, utility functions, management commands, API views, and background tasks. The core logic of replacing Moira with Keycloak seems correct. However, I've identified several issues related to performance, correctness, and maintainability that should be addressed. Key concerns include a hardcoded secret, a highly inefficient implementation for fetching user groups from Keycloak, removal of caching which could lead to performance degradation, and use of placeholder UUIDs in the Keycloak realm configuration.

config/keycloak/realms/ovs-local-realm.json

gemini-code-assist · 2025-08-04T14:51:22Z

.env.example


 # Keycloak configuration
-USE_KEYCLOAK=True
+USE_KEYCLOAK=False


The USE_KEYCLOAK variable is set to False. Given that this pull request is for migrating to Keycloak, this default value seems incorrect for an example configuration. With this set to False, the application will not use Keycloak for authentication. Please consider changing this to True to reflect the intended setup.

USE_KEYCLOAK=True

config/keycloak/realms/ovs-local-realm.json

gemini-code-assist · 2025-08-04T14:51:22Z

keycloak_utils.py

+        try:
+            # Find the user first
+            user = self.find_user_by_username(username)
+            if not user:
+                logger.warning(f"User {username} not found in Keycloak")
+                return []
+
+            # Get all groups
+            all_groups = self.get_groups()
+            user_groups = []
+
+            # For each group, check if the user is a member
+            for group in all_groups:
+                group_members = self.get_group_members(group["id"])
+                if any(member["username"] == username for member in group_members):
+                    user_groups.append(group["name"])
+
+            return user_groups
+        except Exception as exc:
+            logger.error(f"Error querying Keycloak groups for {username}: {exc}")
+            return []


The current implementation of get_user_groups is highly inefficient. It fetches all groups and then iterates through each one to get its members, resulting in N+2 API calls (where N is the number of groups). This can lead to significant performance issues, especially with a large number of groups.

A much more efficient approach is to use the Keycloak API endpoint that directly returns the groups for a specific user: GET /admin/realms/{realm}/users/{user_id}/groups. This would reduce the number of API calls to just 2 (one to find the user, and one to get their groups).

Suggested change

try:

# Find the user first

user = self.find_user_by_username(username)

if not user:

logger.warning(f"User {username} not found in Keycloak")

return []

# Get all groups

all_groups = self.get_groups()

user_groups = []

# For each group, check if the user is a member

for group in all_groups:

group_members = self.get_group_members(group["id"])

if any(member["username"] == username for member in group_members):

user_groups.append(group["name"])

return user_groups

except Exception as exc:

logger.error(f"Error querying Keycloak groups for {username}: {exc}")

return []

try:

# Find the user first

user = self.find_user_by_username(username)

if not user:

logger.warning(f"User {username} not found in Keycloak")

return []

user_id = user["id"]

url = f"{self.keycloak_url}/admin/realms/{self.realm}/users/{user_id}/groups"

response = requests.get(url, headers=self.get_headers())

response.raise_for_status()

groups = response.json()

return [group["name"] for group in groups]

except Exception as exc:

logger.error(f"Error querying Keycloak groups for {username}: {exc}")

return []

ui/utils.py

gemini-code-assist · 2025-08-04T14:51:22Z

keycloak_utils.py

+        if (
+            not hasattr(settings, "KEYCLOAK_SERVER_URL")
+            or not settings.KEYCLOAK_SERVER_URL
+        ):
+            raise ValueError("KEYCLOAK_SERVER_URL setting is missing")
+        if not hasattr(settings, "KEYCLOAK_REALM") or not settings.KEYCLOAK_REALM:
+            raise ValueError("KEYCLOAK_REALM setting is missing")
+        if (
+            not hasattr(settings, "KEYCLOAK_SVC_ADMIN")
+            or not settings.KEYCLOAK_SVC_ADMIN
+        ):
+            raise ValueError("KEYCLOAK_SVC_ADMIN setting is missing")
+        if (
+            not hasattr(settings, "KEYCLOAK_SVC_ADMIN_PASSWORD")
+            or not settings.KEYCLOAK_SVC_ADMIN_PASSWORD
+        ):
+            raise ValueError("KEYCLOAK_SVC_ADMIN_PASSWORD setting is missing")


The validation logic for required settings is repetitive. This can be simplified and made more maintainable by iterating over a list of required setting names.

required_settings = [ "KEYCLOAK_SERVER_URL", "KEYCLOAK_REALM", "KEYCLOAK_SVC_ADMIN", "KEYCLOAK_SVC_ADMIN_PASSWORD", ] for setting in required_settings: if not getattr(settings, setting, None): raise ValueError(f"{setting} setting is missing")

gemini-code-assist · 2025-08-04T14:51:23Z

mail/tasks.py

+    for group_name in video.collection.admin_lists.values_list("name", flat=True):
+        # Find the group first
+        group = keycloak_client.find_group_by_name(group_name)
+        if not group:
+            continue
+
+        admin_lists.append(group_name)
+
+        # Get all members of this group
+        group_members = keycloak_client.get_group_members(group["id"])
+
+        # Extract email addresses from users
+        for member in group_members:
+            recipients_list.add(member["email"])


This block of code can be simplified by using the get_group_members_by_name helper function from keycloak_utils.py. This will make the code cleaner and more consistent with the utilities provided.

Suggested change

for group_name in video.collection.admin_lists.values_list("name", flat=True):

# Find the group first

group = keycloak_client.find_group_by_name(group_name)

if not group:

continue

admin_lists.append(group_name)

# Get all members of this group

group_members = keycloak_client.get_group_members(group["id"])

# Extract email addresses from users

for member in group_members:

recipients_list.add(member["email"])

for group_name in video.collection.admin_lists.values_list("name", flat=True):

group_members = keycloak_client.get_group_members_by_name(group_name)

if group_members:

admin_lists.append(group_name)

for member in group_members:

recipients_list.add(member["email"])

gemini-code-assist · 2025-08-04T14:51:23Z

ui/utils.py


-@lru_cache(1)  # memoize this function
-def get_moira_client():
+def query_user_groups(username: str) -> List[str]:


The type hint for query_user_groups is List[str], but the function returns a set. Please update the type hint to Set[str] to match the return type for clarity and type safety.

Suggested change

def query_user_groups(username: str) -> List[str]:

def query_user_groups(username: str) -> Set[str]:

marslanabdulrauf · 2025-08-07T16:48:21Z

/gemini review

gemini-code-assist

Code Review

This pull request migrates from Moira lists to Keycloak groups. I've identified areas for improvement, mainly concerning configuration issues in the Keycloak realm definition, and some missing mocks in tests. Addressing these will enhance the robustness of the new implementation.

config/keycloak/realms/ovs-local-realm.json

ui/serializers_test.py

pdpinch · 2025-12-01T16:48:58Z

What's the status of this PR?

marslanabdulrauf · 2025-12-02T14:04:52Z

What's the status of this PR?

Its ready for review

mbertrand

I ran into a roadblock while reviewing this, couldn't get keycloak to start properly. Initial errors were caused by dupe ids in the realm configuration. After making each of those unique, I ran into this error:

keycloak-1 | 2025-12-02 16:45:52,919 ERROR [org.keycloak.quarkus.runtime.cli.ExecutionExceptionHandler] (main) Error details:: java.lang.RuntimeException: Script upload is disabled

Which keycloak version do you have installed as the "latest" in docker? It could be that we are on different versions and mine is incompatible with the current keycloak/realm config (something similar happened with apisix for mit-learn: mitodl/mit-learn#2626)

config/keycloak/realms/ovs-local-realm.json

mbertrand · 2025-12-02T16:43:45Z

config/keycloak/realms/ovs-local-realm.json

+          }
+        },
+        {
+          "id": "a1b2c3d4-e5f6-7890-abcd-ef1234567892",


Also a dupe identifier

config/keycloak/realms/ovs-local-realm.json

mbertrand · 2025-12-02T18:26:46Z

ui/management/commands/migrate_moira_to_keycloak.py

        for member in moira_members:
            try:
-                user_result = self.migrate_moira_user(member, moira_list.name, group)
+                user_result = self.migrate_moira_user(member, kc_group.name, group)


There might be many more moira list users than there are current OVS django users, I wonder if this might increase the number of django users significantly., and how long that might take. Has this been tried with the full set of moira lists currently on production?

yeah. I tried to get some stats from my local migration for 2 Moira lists just to get the idea and here are the stats:

mitxpro-video (5 members)

mitxonline-video (109 members)

TIMING BREAKDOWN

Total migration time: 16.33s

MOIRA list fetch time: 4.19s

Group creation time: 4.25s

Member fetch time: 3.11s

User migration time: 8.97s

Average time per user: 0.08s

MOIRA could be a little bit better on the server however as I am using local Keycloak and on server we will be using cloud Keycloak so time may vary there.

Conclusion:
The management command already supports limiting the Moira list for migration so we should migrate the data in batches.

I will try to migrate all MOIRA list locally as well to get better insights

mbertrand · 2025-12-02T18:28:25Z

ui/management/commands/migrate_moira_to_keycloak.py

        return list(moira_lists)

-    def migrate_moira_list(self, moira_list: MoiraList):
+    def migrate_moira_list(self, kc_group: KeycloakGroup):


This management command seems like a good way to create a keycloak group and associated user membership for each moira list currently in the OVS system. But how will new groups and changing user memberships be maintained going forward?

If keycloak groups are meant to replace moira lists entirely, should there be a UI for authorized users to create new groups and update user memberships for all groups? I think professors can create/maintain moira lists for their own courses, so it seems like we'd need a way to let them to do the same with keycloak groups?

yes, at that point when I worked on this I didn't have much context about how OVS was being used along with MOIRA list but definitely if we go through this route, we need some way for professors to create/manage groups.

And in our recent discussion on how we are going to integrate Keycloak in OVS, using touchstone as LDAP in Keycloak (I don't know much about this) so I am not sure if we really need this migration.

Cc @blarghmatey

gitguardian · 2025-12-03T10:10:50Z

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

^{_{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}}

marslanabdulrauf · 2025-12-03T12:36:48Z

I deleted the existing container and I am getting the same error now, looks like new versions have different syntax to support script upload.
I am looking into it

gemini-code-assist bot reviewed Aug 4, 2025

View reviewed changes

marslanabdulrauf mentioned this pull request Aug 4, 2025

feat: keycloak service added with pre-defined realm and configurations #1289

Open

marslanabdulrauf force-pushed the marslan/6821-moira-to-keycloak branch 3 times, most recently from 0e5feee to 3708d83 Compare August 7, 2025 12:13

gemini-code-assist bot reviewed Aug 7, 2025

View reviewed changes

config/keycloak/realms/ovs-local-realm.json Outdated Show resolved Hide resolved

config/keycloak/realms/ovs-local-realm.json Show resolved Hide resolved

ui/serializers_test.py Outdated Show resolved Hide resolved

ui/serializers_test.py Outdated Show resolved Hide resolved

marslanabdulrauf marked this pull request as ready for review August 8, 2025 12:37

marslanabdulrauf added the Needs Review label Oct 27, 2025

mbertrand self-assigned this Dec 2, 2025

mbertrand requested changes Dec 2, 2025

View reviewed changes

mbertrand reviewed Dec 2, 2025

View reviewed changes

marslanabdulrauf force-pushed the marslan/6821-moira-to-keycloak branch from f0f265a to 2c84678 Compare December 3, 2025 10:10

marslanabdulrauf force-pushed the marslan/6821-keycloak branch from e5afdd4 to 4e5fe0f Compare December 3, 2025 10:25

marslanabdulrauf force-pushed the marslan/6821-keycloak branch from 9fb6ff5 to e9071ef Compare December 3, 2025 20:44

marslanabdulrauf force-pushed the marslan/6821-moira-to-keycloak branch from 2c84678 to ecfe7af Compare December 4, 2025 12:04

marslanabdulrauf force-pushed the marslan/6821-keycloak branch from a36bbc1 to 789bc07 Compare December 16, 2025 15:49

marslanabdulrauf added 7 commits December 16, 2025 21:16

fix: realm configs upgraded and python-social-app-django added

b3260c0

feat: Moira lists usage replaced with Keycloak groups

936526e

refactor: keycloak refactoring and management command fixes

457c992

test: util tests fixed

417985b

test: serializers test fixed

5ffb9d8

feat: table renamed and tests fixed

28c2e28

fix: keycloak client fixed

bbe3ae6

marslanabdulrauf force-pushed the marslan/6821-moira-to-keycloak branch from ecfe7af to bbe3ae6 Compare December 16, 2025 16:17

marslanabdulrauf added 2 commits December 17, 2025 17:16

fix: realm duplicate id fixed

0941b6b

fix: empty members and mailList handled, time matrics added

17be8e7

	def query_user_groups(username: str) -> List[str]:
	def query_user_groups(username: str) -> Set[str]:

Moira lists to Keycloak groups #1324

Are you sure you want to change the base?

Moira lists to Keycloak groups #1324

Uh oh!

Conversation

marslanabdulrauf commented Aug 4, 2025

What are the relevant tickets?

Description (What does it do?)

How can this be tested?

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist bot Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gemini-code-assist bot Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gemini-code-assist bot Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

marslanabdulrauf commented Aug 7, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pdpinch commented Dec 1, 2025

Uh oh!

marslanabdulrauf commented Dec 2, 2025

Uh oh!

mbertrand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mbertrand Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mbertrand Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

marslanabdulrauf Dec 17, 2025

Choose a reason for hiding this comment

TIMING BREAKDOWN

Uh oh!

mbertrand Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

marslanabdulrauf Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

gitguardian bot commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

️✅ There are no secrets present in this pull request anymore.

Uh oh!

marslanabdulrauf commented Dec 3, 2025

Uh oh!

gitguardian bot commented Dec 3, 2025 •

edited

Loading