atomic database transaction in process_access_token

[F0rcey]

Creates a atomic database transaction in process_access_token, to avoid creating and commiting a user without attributes, if validation of attributes fails.
If, for example, the email of a user as a unique constraint, create_users would create a user with only username and commits it to the database, then fails in update_user_attributes.
This change does a rollback and does not commit "empty" users to the database

[tim-schilling]

Are these changes necessary?

[F0rcey]

Just tested with only full_clean, only save and without both: at least the save() call is necessary.
I suspect the explicit save() call is necessary because of the atomic transaction. Without it the functions auto commited the changes to the database.
Omitting the save() call saves the users without attributes (valid and invalid alike)

With the explicit call to save(), invalid users get rolled back and no database entries without attributes are created, while valid users get added as expected.

[tim-schilling]

My confusion is that you're adding a full_clean and save and not removing the other. https://github.com/snok/django-auth-adfs/pull/394/files#diff-3a0b0facfd04fac71955cf0afe255517022c705b67fcbe91f75499d6baf8a0baR216-R217 Something about this doesn't sit right.

I don't disagree we should use a transaction, however, now we have to decide if the post_authenticate signal should be emitted within that block or after. I believe after is the right choice. We'd probably want to make this at least a minor version change.

[F0rcey]

Fair point. I more or less tried what worked and went with that. Just adding the atomic transaction block still commited some rows to the database for invalid users. I guess adding a full_clean might not be wrong, but removing it didn't change the wanted outcome.
I would also say that emitting the post_authenticate after the atomic block is correct.

Let me know if I can be of any help with testing or other changes :) I've added this library to a taiga board backend, which is where this behavious caught my attention.