search

SuperCoopBerlin / tapir

Member & shift management system for our cooperative supermarket, SuperCoop Berlin eG
GNU Affero General Public License v3.0
31 stars 22 forks source link

WIP Switch to django-auth-ldap #548

Closed Theophile-Madet closed 2 weeks ago

Theophile-Madet commented 2 weeks ago

I'm trying to switch to another library for logging into ldap, that would hopefully let us upgrade python and django to current versions.

For now click testing seems to work, I'm starting on the tests.

Theophile-Madet commented 2 weeks ago

Hey everyone, This PR is now ready. It's quite big! Have a look if you feel like it, but it's not the easiest to understand :s

It's about switching from django-ldapdb to django-auth-ldap. django-ldapdb was giving us classes similair to django models, where we could do stuff like LdapGroup.objects.filter(...). Authentication, password changes... was built on top of that in classes that Leon wrote at the beginning of Tapir. It was, if I remember correctly, the last thing that was blocking us from upgrading Django.

django-auth-ldap is a bit less friendly, it handles only authentication. For manipulation groups and passwords, we now rely on python-ldap, which is the library that django-ldapdb and django-auth-ldap are both built on top of. Calls like add_s(), modify_s() are on python-ldap. It's not as intuitive to read and write, but at least it's an updated library.

An unexpected side effect is that tests are now about twice as fast to run, so that's nice! Another nice side effect is that the buggy migrations that were created in the accounts app everytime we did makemigrations don't show up anymore 🎉

I deployed this branch to the test instance just now. I think I'll wait until tomorrow to merge it into master and deploy to prod, and ask on Slack for feedback in case members have trouble logging in.

I'm actually not sure if all this will let us update Django, because I don't remember what the blocking point was. After this branch is deployed to prod, I'll start working on upgrading to Django 5 and Python 3.12.

If this all works, we won't need to setup Keycloak at all, at least for now.

kerstenkenan commented 2 weeks ago

Hi @Theophile-Madet, thanks so much for this update 👍👍👍! This will help us surely of lot in the future and gives a lot more security. I looked over the code, but of course I can give you no advice ;). Can I help you otherwise to check things on test1?