drop unneeded unique on public_key (#594)

[jpaniagualaconich]

Description

Drops the unique constraint on Webauthn.public_key.

Modifies two_factor_webauthn migration 0001 so public_key doesn't start with a unique constraint, otherwise it'll fail if the db is MariaDB.

Adds 0002 to drop the unique constraint. It'll still run fine even if there is no unique constraint to drop, resulting in a no-op in that case.

Motivation and Context

Fixes #594 .

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

• [x] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• [x] My code follows the code style of this project.
• [ ] My change requires a change to the documentation.
• [ ] I have updated the documentation accordingly.
• [ ] I have added tests to cover my changes.
• [x] All new and existing tests passed.

[codecov[bot]]

Codecov Report

Merging #610 (dc726e4) into master (fd07d82) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #610   +/-   ##
=======================================
  Coverage   95.46%   95.46%           
=======================================
  Files          74       74           
  Lines        3198     3198           
  Branches      358      358           
=======================================
  Hits         3053     3053           
  Misses        116      116           
  Partials       29       29           

Impacted Files	Coverage Δ
two_factor/plugins/webauthn/models.py	91.66% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[jpaniagualaconich]

OK, ready for another round.

Admittedly a not-super-elegant solution but I like its simplicity.

[jpaniagualaconich]

Been there, tried that. Tinkering with CreateModel, state_forwards and database_forwards means too much wading through the entrails of the migration infrastructure.

Also, I didn't want to depend on an undocumented vendor attribute.

Anyway, let me propose a somewhat different solution that leaves CreateModel and AlterField alone and instead modifies Migration.apply and unapply directly and is perhaps less hacky, at least in my opinion.

[claudep]

616 should solve the test failures.

[jpaniagualaconich]

I'm afraid I don't fully understand your question. Avoid the second migration in which sense?

Applying 0002 for MySQL results in a no-op, so for practical purposes we are avoiding the second migration already.

[claudep]

My (maybe stupid) question was that if we fixed the MySQL issue, can we keep the unique index?

[jpaniagualaconich]

We want model state to be the same in all cases, MySQL or not, after 0002 is applied so a future 0003 migration won't need to do acrobatics like 0001 and 0002 are forced to do now.

When the executor builds the state of the model it uses Migration.mutate_state which doesn't get schema_editor, meaning that we can't change things for MySQL since we don't know the database.

As a result, the non-MySQL versions of 0001 and 0002 are used. The consequence of this is that the state of WebauthnDevice is wrong for MySQL after mutate_state for 0001, getting fixed only after mutate_state for 0002.

In any case, having that pesky unique=True was totally unneeded in the first place.

[claudep]

That makes a lot of sense. Can you please rebase your PR now? Tests should hopefully pass.

[claudep]

Current error: File "...two_factor/plugins/webauthn/migrations/0001_initial.py", line 60, in Migration operations = [create_webauthn_device_model(True)] TypeError: 'staticmethod' object is not callable

[jpaniagualaconich]

deprecated codecov gave up the ghost?

[claudep]

Looks so... I will not regret it.

[claudep]

Looks like #619 fix this failure.

[claudep]

Could you please rebase on master ? #619 was merged.

[jpaniagualaconich]

done. Looks ready for the merge.