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

[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 

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@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.

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

[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


On Sat, Mar 5, 2022 at 12:54 PM Florian Apolloner 
wrote:

> 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
>
> --
> 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-developers+unsubscr...@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
> 
> .
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CAAC-pBMUwpHDmE%3D_CoWBpJtRhA89sWVKmgADr4i_LCNqkJAQUg%40mail.gmail.com.

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

[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 

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/a5cc89c2-1dbf-4a97-87e6-8744e8e5b64fn%40googlegroups.com.