Signals and Documentation

[DylannCordel]

Hi,

Since pre_user_save and post_authenticated are not any more used, I think it should be great to delete signals.py to avoid a working website wich was using signals to performs some actions ?

[peppelinux]

I completely agree but at the same time I'm looking for contributor that would reintroduce these (and document this feature) standing on what already documented in django and what would be the best practices for djangosaml2

I'm looking for contributor for that, would you like to join in this?

[DylannCordel]

Yes we (Webu) can join it.

I think there are 2 approaches (not exclusive):

• no signal, but extends the backends and views + override urls (like in the current master branch): need doc for :
  • how to have our own backend with examples
  • which "hooks" we can override in backend and need some code modification (for what I need, I must override _update_user because current "hooks" are not enough)
  • how to have our own views (like AssertionConsumerServiceView: extend it, change urls, etc.) with examples
  • which "hooks" we can override in those views
• send djangosaml2 signals like before + add doc

I think both approaches have advantages and depend on what you want to do. For exemple, it's simpler to have multiple independant app's which add custom code via signals. But for dependant apps, it's stronger to use hooks via multi Mixins inheritance for final Backend / View.

What do you think about it ?

[peppelinux]

regardings signals:
we need a general review of the current approach and a documentation for that

regarding hooks we have two strategies:

1. inherit AssertionConsumerServiceView and use urls.py to get it in. This commonly used for additional checks/validation and error handling regarding SAML2 Response.
2. inherit Saml2Backend as authentication backeds (suggested for internal user attributes, rewrites, storage, model ...)

as alternative we could move to a better generalizable model, similar to this:
https://github.com/UniversitaDellaCalabria/uniCMS/blob/a317c0d71b2c9ef6f2f9992f59eeb46f744522cf/example/unicms/settingslocal.py#L306

here we have some registered hooks (that could be also external django apps).
Each hook MUST accept a predefined (by-design)set of attributes, example: request, ava ...
In the core of djangosaml2 (AssertionConsumerServiceView and Saml2Backend) we should only register the calls of the configured hooks, similar on what did here as signals

https://github.com/UniversitaDellaCalabria/uniCMS/blob/main/src/cms/contexts/signals.py

or adding the calls in the class methods.

Probably the settings HOOKS would be the smartest thing if you agree

[peppelinux]

Hi @DylannCordel
Have you found something interesting to start with?

Which approach would you prefer?

[DylannCordel]

Hi @peppelinux

IMHO, be able to inherit some classes (views, backends...) which are designed to add some code execution at specific moment to change/add some behaviours is a must have. In the other hand, signals are also great to perform some actions which are independant from what SAML does, but just need to be executed at the right moment and get access to informations.

Concerning the signal registration via a setting, I'm not sure. I would prefer that third-party apps stay autonomous to register receivers once they are added in INSTALLED_APPS. If a signal is "order dependant" from th other in it's execution, I think it must not be a signal but an extend of the class where code is run.

In conclusion, I think both approaches are complementary (code designed to be overrided + signals) and will allow developpers to get a way for every thing they want to do.

I'll create a wip PR with just the documentation and if you are ok with what is proposed, I'll implement it.

[oliwarner]

This has been handled in exactly the wrong way. Removing signalling callers without removing the signals themselves means there are chumps like me doing upgrades and some time later finding out that their whole system has fallen apart and not being able to find out why until they start digging around in ACS view code.

Why doesn't AssertionConsumerServiceView.post_login_hook(..) just send the post_authenticated signal by default? That's next-to-zero effort to keep (some) systems working. Getting pre_user_save hooked back in via the class would be useful too but that takes a little more thought.

But please, just do something. If you remove the signals it will break code and that is a good thing! Means developers get the opportunity to notice this and fix things. Or you could put deprecation warnings (or throw errors) in the signals.py file (that trigger when imported). But leaving it like this is hurting users.

[peppelinux]

Nice shot @oliwarner

this issue and the relative PR seems to be in pause.
Would you like to purpose a PR with the post_login_hook method in ACSView?

We can include and document this feature in the next release 1.4.0 of djangosaml2 if you agree

[oliwarner]

@peppelinux It's hard to see how to best unravel 343c7e4

If signals are going to carry on existing, reverting most of it seems most sensible. If they're not, I do think deleting signals.py (and breaking downstream systems so they have to fix things) is the healthier option.

[peppelinux]

I'll proceed deleting signals.py as soon as possible

[peppelinux]

anyway @oliwarner we have this very good work from @DylannCordel here
https://github.com/IdentityPython/djangosaml2/pull/292/files

before deleting singlas.py I'd like to give a change to this work.
we could have both post_auth method and signals, each implementer would decide if inherit the class and overload the method or registering a new signal

having said this I think that we should decide together if going ahead with signals and complete that PR or clean up the source tree from signals.py

[oliwarner]

Definitely. I really appreciated signals as a way to hook this behaviour. They were much easier than inheriting a CBV, overriding methods within that, pointing an extra URL to your new overridden view in such a way to override the old one. That's all a pain in the bum we can do without (unless you really want to). Signals are way better.

My problem was the current state of things. While signals exist and aren't being called, there will be djangosaml2 users whose listeners aren't being actioned and important business logic calls aren't happening. The PR from @DylannCordel looks good.

The signature on pre_user_save has changed. Does Django detect this? If it doesn't, I'd suggest renaming it. That way it'll force any people using the old signals to update their code.

[peppelinux]

@oliwarner would you like to give a patch on this and complete the WiP of @DylannCordel with your very reasonable ideas?
I can give you a modest revision and a certain merge :)