devilry / trix2

Next generation Trix. Detailed task control and statistics app for better learning outcome.
BSD 3-Clause "New" or "Revised" License
2 stars 3 forks source link

User migrations when switching authentication method #111

Closed torgeirl closed 1 year ago

torgeirl commented 1 year ago

While the federated authentication (#88) itself works, it seems it doesn't combine new login sessions to existing users. This could be due to the our email notation changing in the years since the authentication backend was implemented. With LDAP we used a username@default.domain.com (always legal) notation while the users primary email often where username@department.domain.com. The default address is now simply username@domain.com, and this doesn't seems to mash with existing users, creating duplicate accounts.

Perhaps there is an Allauth configuration that solves this? Or perhaps we need to update all users' email suffix and at the same time add their account_emailaddress?

torgeirl commented 1 year ago

As mentioned I'm fairly certain this worked when we initially tested 3.0.0, before the default email provided by Dataporten changed. Could the issue be a default matching pattern that assumes email addresses on the form username@dep.uni.domain?

Some scenarios:

If there is a default matching pattern it probably should be moved from defaults to a setting we can overwrite in case the pattern changes again.

Levijatan commented 1 year ago

Can not find any settings to adjust to have the desired effect. The problem seems that the trix user uses the email as the unique identifier of the user. Probably should change it to user_id received from the Dataporten login call and fall back to the username part of the email if user_id is not set.

torgeirl commented 1 year ago

Can not find any settings to adjust to have the desired effect. The problem seems that the trix user uses the email as the unique identifier of the user. Probably should change it to user_id received from the Dataporten login call and fall back to the username part of the email if user_id is not set.

For context, with LDAP with just mapped the username to username@default.domain.com and I believe email was used as key (unable to check right now). That's why I assumed the OAuth login with Dataporten made an assumption about the email since it worked before our email pattern changed.

We're fine with keeping the email as unique identify, but since changing existing users' email didn't work I assume something else needs to be changed in an migration script (if we select to go down that path).

Levijatan commented 1 year ago

So I have added a call to allauth.account.utils.sync_user_email_addresses on save for users to correctly populate the UserEmail objects that allauth uses when you change the email of a user inn the admin interface. And I have enabled the allauth authentication backend inn the default settings file to make sure allauth is working as intended. So thinking maybe make a migration that goes through all users and make sure the user has a UserEmail object for the old mail and add a UserEmail object for the new default addresse

torgeirl commented 1 year ago

Would this change block us from switching back to LDAP authentication in the future? I don't think its likely we need it, but we do want to keep things fairly deployment agnostic and if need by its nice to keep it as an option unless it takes too much time keeping it around.

If not, could you make another beta release?

Levijatan commented 1 year ago

I will fix it so it can work with the LDAP authetication, it is an easy fix. And then I'll make a beta release.

Levijatan commented 1 year ago

The new release is done.

torgeirl commented 1 year ago

Not entirely sure what's wrong, but neither existing or new users are able to log in with the latest release.

I assume this is unrelated to our issue, but to be sure we catch anything previously manuelly fixed I reverted our test database to match our production before upgrading to this release, and then migrated to 3.0.0b6 (no issues). I then added edited django_site and added socialaccount_socialapp and socialaccount_socialapp_sites manually.

We get an AssertionError: EmailAddress has no user. for line 252 in allauth/account/utils.py (the setup_user_email function it seems).

Levijatan commented 1 year ago

Would it be possible to get a complete stack trace of the error? At least the full error log? But it looks like you might have some bad data inn your test database. I can not reproduce the error at this moment.

torgeirl commented 1 year ago

Would it be possible to get a complete stack trace of the error?

Check your email. :wink:

Levijatan commented 1 year ago

Yeah it was not the best place for the sync_user_email_addresses call. But I have now created an socialaccountadapter to handle the connection between local and social accounts, based on how it is handled inn devilry. And I have made a migration that changes all 'default.domain.com' emails to 'domain.com' and makes sure all users have the corresponding 'EmailAddress' objects.

Levijatan commented 1 year ago

I have made a b7 pre-release

torgeirl commented 1 year ago

I have made a b7 pre-release

Unfortunate, the migration fails:

Begin migration ...
Operations to perform:
  Apply all migrations: account, admin, auth, contenttypes, sessions, sites, socialaccount, trix_core
Running migrations:
  Applying trix_core.0006_auto_20221121_1419...
Traceback (most recent call last):
  File "/app/manage.py", line 7, in <module>
    execute_from_command_line(argv)
  File "/usr/local/lib/python3.8/site-packages/django/core/management/__init__.py", line 364, in execute_from_command_line
    utility.execute()
  File "/usr/local/lib/python3.8/site-packages/django/core/management/__init__.py", line 356, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/local/lib/python3.8/site-packages/django/core/management/base.py", line 283, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/usr/local/lib/python3.8/site-packages/django/core/management/base.py", line 330, in execute
    output = self.handle(*args, **options)
  File "/usr/local/lib/python3.8/site-packages/django/core/management/commands/migrate.py", line 202, in handle
    post_migrate_state = executor.migrate(
  File "/usr/local/lib/python3.8/site-packages/django/db/migrations/executor.py", line 115, in migrate
    state = self._migrate_all_forwards(state, plan, full_plan, fake=fake, fake_initial=fake_initial)
  File "/usr/local/lib/python3.8/site-packages/django/db/migrations/executor.py", line 145, in _migrate_all_forwards
    state = self.apply_migration(state, migration, fake=fake, fake_initial=fake_initial)
  File "/usr/local/lib/python3.8/site-packages/django/db/migrations/executor.py", line 244, in apply_migration
    state = migration.apply(state, schema_editor)
  File "/usr/local/lib/python3.8/site-packages/django/db/migrations/migration.py", line 129, in apply
    operation.database_forwards(self.app_label, schema_editor, old_state, project_state)
  File "/usr/local/lib/python3.8/site-packages/django/db/migrations/operations/special.py", line 193, in database_forwards
    self.code(from_state.apps, schema_editor)
  File "/usr/local/lib/python3.8/site-packages/trix/trix_core/migrations/0006_auto_20221121_1419.py", line 19, in fix_user_email
    sync_user_email_addresses(u)
  File "/usr/local/lib/python3.8/site-packages/allauth/account/utils.py", line 345, in sync_user_email_addresses
    if email and not EmailAddress.objects.filter(user=user,
  File "/usr/local/lib/python3.8/site-packages/django/db/models/manager.py", line 85, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/django/db/models/query.py", line 787, in filter
    return self._filter_or_exclude(False, *args, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/django/db/models/query.py", line 805, in _filter_or_exclude
    clone.query.add_q(Q(*args, **kwargs))
  File "/usr/local/lib/python3.8/site-packages/django/db/models/sql/query.py", line 1250, in add_q
    clause, _ = self._add_q(q_object, self.used_aliases)
  File "/usr/local/lib/python3.8/site-packages/django/db/models/sql/query.py", line 1273, in _add_q
    child_clause, needed_inner = self.build_filter(
  File "/usr/local/lib/python3.8/site-packages/django/db/models/sql/query.py", line 1179, in build_filter
    self.check_related_objects(field, value, opts)
  File "/usr/local/lib/python3.8/site-packages/django/db/models/sql/query.py", line 1076, in check_related_objects
    self.check_query_object_type(value, opts, field)
  File "/usr/local/lib/python3.8/site-packages/django/db/models/sql/query.py", line 1052, in check_query_object_type
    raise ValueError(
ValueError: Cannot query "User object": Must be "User" instance.
Levijatan commented 1 year ago

Fixed the migration. New version is up

torgeirl commented 1 year ago

It works for new users again, while existing users are meet by a signup page:

Screenshot from 2022-11-23 12-21-06

Clicking the button yields an error: «An account already exists with this e-mail address. Please sign in to that account first, then connect your Dataporten account.»

Levijatan commented 1 year ago

Added an override to is_auto_signup_allowed to allow auto signup for existing users. Hopefully that is the last piece of the puzzle. Created a 3.0.0b9 release.

torgeirl commented 1 year ago

Existing users still get the signup message. As far as I can tell, there are still a few differences between new and existing users:

I've manually updated the primary for an existing user from False till True, and that didn't solve the issue (by it self).

My guess is that something still is missing for merging existing users with a new login session, and that's why it defaults to the signup form.

Levijatan commented 1 year ago

It was missing the settings to actualy use the custom socialaccountadapter. b10 has been released

torgeirl commented 1 year ago

That solved it! :tada: