Skip to content
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

Switch over to a custom user model #207

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jkachel
Copy link
Collaborator

@jkachel jkachel commented Feb 4, 2025

What are the relevant tickets?

Closes mitodl/hq#6597

Description (What does it do?)

Switches the app to use a custom user model, and makes some changes to its copy of the APISIX middleware to handle that (and fix a bug or two).

This adds a user model with a global_id, which exists to store the SSO ID for the user (Keycloak's UUID, for now). It also changes the APISIX middleware:

  • To properly create new users - i.e., make sure the fields are all filled, including full name, username, and global_id, and sets the is_active flag properly
  • To consider the is_active flag for existing users - if we've turned the user off in Django, they should not be allowed in

The existing app models were changed to reference settings.AUTH_USER_MODEL for FKs where applicable. I didn't see any additional places where we were referencing the User model directly. This also updates the User Admin in Django Admin to work with the additional fields that are in the new User model.

This also fixes a couple of minor things:

  • The Postgres DB now has a persistent storage volume. (Not sure how this got missed, but it did.)
  • The Postgres DB now pulls in a SQL script to create the Keycloak DB automatically when there's no database. You will not have to manually create this when using the pack-in Keycloak.
  • The Dockerfile now will copy in any mitol_django packages that you've built locally so you can test them more easily. (You still have to update the source in pyproject.toml before they'll be used - this just means you can drop the resulting build tarball in the project root and it'll be there when you rebuild the image.)

How can this be tested?

All automated tests should pass, and you should be able to use the system as per usual.

Specifically: you should be able to check out successfully. For an admin user, you should be able to promote the user and have them be able to get to the Django Admin successfully. Deactivating a user should deny them access from the system. API calls should work.

Additional Context

Some of these changes are a result of the work in learn-ai - the APISIX middleware stuff will eventually be moved to a reusable ol-django app so this is just further refinement.

Adds in the user model and makes sure the models that reference the user model use the `settings.AUTH_USER_MODEL` method. Creating the migration at this point generates a bunch of errors about groups and permissions, so need to resolve that next - this code won't work as is.
This works best if you trash your existing database. (We're early enough that that shouldn't be a big deal.)

- Finalizing changes to the user model and squashing the users migrations down to one
- Fixes for the APISIX middleware to check that the user is active, create the user properly when it has to
- Set a default for global_id - if we create a manual user, the field needs to be populated, and it's a UUID so no harm in just filling it as per usual
- Update the docker_compose env to actually put the Postgres database in a persistent volume, imagine that!
- Update the docker_compose env to pull in an init script for Keycloak so you don't have to manually do it
- Update the Dockerfile to pull in mitol_django packages you might have built and added manually - no changes to the pyproject.toml for this, you get to do that yourself still
- Update gitignore to ignore the mitol_django manual packages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant