[17.0][IMP] auth_saml: user provisioning on login

[vincent-hatakeyama]

• custom message when response is too old
• avoid using werkzeug.urls method, they are deprecated
• add missing ondelete cascade when user is deleted
• attribute mapping is now also duplicated when the provider is duplicated
• factorize getting SAML attribute value, allowing using subject.nameId in mapping attributes too

This is inspired by #655.

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[lorenzoallegrucci]

LGTM, @vincent-hatakeyama could someone merge it?

[vincent-hatakeyama]

The OCA process requires two reviews before merging can be done.

You are welcomed to leave a review.

[arnaudlayec]

Hello,
It looks very good to me, thanks for the interest of the initial PR.
I just don't understand the feature of re-activating an archived user, I think it could be a security issue, unless I'm missing something.

Do you plan to (back)port it, to v16.0 / v18.0? I could help on v16.0 at least

[arnaudlayec]

Was is the use-case for this feature? (activating a disabled user)
I think it can be a security issue. Let's say I don't way a user to login anymore, but I want to keep its detail in my database: I usually archive it. But I don't want her/him to be able to authenticate back thanks to the this auto-provisioning feature.
I think we should consider archived users like non-existing user.

[vincent-hatakeyama]

The use case:

The identity provider list users and is up to date. If a user is not in the company, the user is disabled here.
Eventually, an odoo admin can disable users, but it is not mandatory ; or maybe another system is in place to disable them once disabled in the identity provider. If there was a SCIM implementation in odoo, that would be done that way (and in that case, that feature would be useless).
After some time, the employee joins back the company and their user is reactivated in the Identity Provider. In that case, the odoo user should be reactivated. As the login is unique, it is probably impossible to create a new user.

If you have a use case where the user should be not be activated again, that behaviour could be an option.
In any cases, I should update the description of the feature to reflect that point.

[arnaudlayec]

Thanks for the explanation, I understand now.
I agree with your use-case, except about who (which tool) should re-activate the user. I think it's up to the tool or person who disabled it in the first place to re-activate it when needed, and also to warn the administrator about the synchronization in place. It also still think it's a security issue to automatically re-active archived users here, since we don't know who/what archived it in the 1st place.
I feel like this re-activation feature should/could be managed by a SCIM implementation or any other identity-synch solution. It could be managed by this SAML module, but maybe by the claims?

To explain my use-case, it was a bit opposite to your standard. It is a small-to-medium company of 50-employees which uses Microsoft 365 for emails and identity management. The HR management wants sometime to disable user in Odoo directly (not in external identity provider) for manual-allowed control on the ERP, independently of email/identity in M365 because ERP access is sensitive. I agree controlling access to the ERP should be done by managing groups in identity provider of who has/has not access it, but this is too complex for this small/medium company.
=> So this is a use-case where active field modified in Odoo takes precedence over the user status in external identity tool.

Another thought:
Syncing active field from external identity provider is a real choice, at the discretion of the administrator.
Not tested, but: if a Odoo user is active Odoo and disabled in identity provider, I think this is the identity provider who will block the authentication request, anyway of the active field in Odoo, right?
=> So I'm really wondering the need of updating active field in hard code in Odoo.

If we want to manage the update of active field in this SAML module, should it be supported through the claims? I haven't tested/though about it hardly enough but I guess it would imply:

• active_test=False some places in the code
• early update of active field if managed in the claims and if modified
• check user.active boolean before authenticating it, or raise AccessDenied

Sorry for such many words, I hope I'm not adding complexity.
I'm not an identity-management expert, so if your use-case seems like a standard in the identity industry, go for it. Indeed a README.md update seems needed in that case.

[vincent-hatakeyama]

Identity providers are meant to be the source of the users and their rights. Having it otherwise is not standard practices. There is still a missing piece in auth_saml (also missing from auth_oauth) to match groups in the identity providers and groups in the application. This could be included in a SCIM implementation instead of putting it in the modules.
This implementation is a workaround the lack of SCIM implementation to create the user. There is still the issue of giving the correct groups to users.

Disabling users can either be done manually, with a separate process (which is done in the company that uses the feature in this PR) or not done at all.

The simplest way to handle the various cases is to introduce a new boolean in a provider to indicate if deactivated user should be activated again. I’m going to make the default to not re-enable disabled users.

[arnaudlayec]

Thanks for listening, I also agree
This lets some area of improvements for future work:

• manage group synchronization for permissions
• and maybe support archiving/re-activation of users through claims

[vincent-hatakeyama]

Concerning the port to 18, there was one (#823) but it now lacks the latest fixes made here. I plan on porting the feature in 18.0 if its merged.
I do not plan to backport it in 16.0.

[arnaudlayec]

Ok after above discussion, thank you!