RFC #33561 -- Synchronize user attributes on every authentication with RemoteUserBackend

[Adrian Torres Justo]

Hello all,

I'd like to get your feedback and comments regarding the ticket mentioned in the subject line: adding a mechanism/hook to the RemoteUserBackend that allows for synchronization of user attributes between the remote system and the django server on each authentication attempt.

Synchronizing data between the remote system and the django server is useful for obvious reasons, however the ticket was marked as WONTFIX arguing that the same can be achieved by overriding authenticate().

While the behavior is similar, it is not the same: My implementation (see linked patch in Trac) synchronizes data right before the return line of the authenticate() method, which checks whether the user can authenticate by calling user_can_authenticate() whose result can be influenced by the synchronization, which is why I did it right before it.

You could argue that then one can simply override user_can_authenticate() and call the synchronization method from there, and you would be right, however this is not obvious to RemoteUserBackend implementors and feels more like a hack than anything else, not to mention you wouldn't know this without reading the source code, which is what I had to do in order to implement this functionality in another project (and what led me to submit the patch in the first place).

Ultimately, the argument could be used for the clean_username hook too, why have such a hook if one can simply override authenticate() and perform the cleaning right before calling authenticate()?

Another idea would be to use configure_user() for both initial configuration and synchronization by passing an extra parameter "created" to it, and calling it just before the authenticate method's return line, but I figured this change would be more disruptive for existing implementations.

Thank you in advance for your inputs,

Adrian

[Florian Apolloner]

Hi Adrian,

I agree this would be nice to have.

On Friday, March 4, 2022 at 8:03:09 PM UTC+1 ator...@redhat.com wrote:

Another idea would be to use configure_user() for both initial configuration and synchronization by passing an extra parameter "created" to it, and calling it just before the authenticate method's return line, but I figured this change would be more disruptive for existing implementations.

I do prefer that approach. It is not more disruptive (or at least only marginally) and only means more work (backwards compat warnings) when implementing this. We can easily inspect the existing signature and only pass the boolean when supported and in the backwards compatibility period simply do not support created=False when the user didn't change their configure_user signature. I'd hate seeing two methods that basically do the same.

Cheers,

Florian

[Adrian Torres Justo]

Hey Florian,

First of all, thank you for the feedback and I'm glad you agree that the feature would be nice to have :)

I'm willing to implement whichever version people agree on since I do think the feature will be useful, but I do think that having separate methods is clearer, simpler, as well as easier to maintain and extend:

- Existing RemoteUserBackend implementations won't need to change signatures whenever backwards compatibility is removed

- RemoteUserBackend implementations won't need to do anything in order to support Django versions in which the feature doesn't exist (e.g. 3.9) and versions in which the feature exists and is not backwards-compatible (e.g. 5.1)

- The code footprint within Django, not counting documentation and tests, is like 3 LOC

- Anyone who wants to extend a RemoteUserBackend implementation can easily and cleanly extend/replace the synchronization and initial setup independently of each other, if everything is done in the same method this can get messy

That last point might lead implementors to define their configure_user like so:

def configure_user(self, request, user, created):

    if created:

        user = self.initial_configuration(request, user)

    user = self.synchronize(request, user)

    return user

Which is the same as having two separate methods for initial configuration and synchronization, but with extra steps.

Have a good weekend,

Adrian

--
You received this message because you are subscribed to a topic in the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/django-developers/dn_E9IzayZA/unsubscribe.
To unsubscribe from this group and all its topics, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/9db3b605-a8fc-40ab-a318-0ff1e7a76eb3n%40googlegroups.com.

[Florian Apolloner]

Hi Adrian,

On Saturday, March 5, 2022 at 5:13:14 PM UTC+1 ator...@redhat.com wrote:

- Existing RemoteUserBackend implementations won't need to change signatures whenever backwards compatibility is removed

- RemoteUserBackend implementations won't need to do anything in order to support Django versions in which the feature doesn't exist (e.g. 3.9) and versions in which the feature exists and is not backwards-compatible (e.g. 5.1)

While this is true, the "migration" path for implementors is simply to change the function to something like:

```

def configure_user(self, request, user, created=False):

    if not created: return

   … do whatever you already did

```

and it will stay compatible with current behavior and the new behavior.

 

- The code footprint within Django, not counting documentation and tests, is like 3 LOC

I do understand that, but I don't think it is as simple as that. With the separated methods (unless we pass created to synchronize_user as well) you'd do synchronization always even when the user was just created even though configure_user could take care of that in one go.

- Anyone who wants to extend a RemoteUserBackend implementation can easily and cleanly extend/replace the synchronization and initial setup independently of each other, if everything is done in the same method this can get messy

I do not really think this will be the case since those methods are empty by default… Those backends are so simple that at some point one can simply write their own if subclassing might become to much of a hazzle.

 

def configure_user(self, request, user, created):

    if created:

        user = self.initial_configuration(request, user)

    user = self.synchronize(request, user)

    return user

Which is the same as having two separate methods for initial configuration and synchronization, but with extra steps.

I do not think this will be common though. I rather think that usually one would do the same thing on creation and on sync.

 

Have a good weekend

Thanks, you as well!

Florian

[Adrian Torres Justo]

Alright, I'm still not 100% convinced it wouldn't be best having them as separate methods, but for the sake of moving things along I'll implement it.

One thing that's not quite clear: you mentioned adding a backwards compatibility warning, is this the same as a RemovedInDjangoXXWarning? Should I follow the documentation at [1] for this implementation?

[1] https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/submitting-patches/#deprecating-a-feature

Cheers,

Adrian