[15.0][FIX][IMP] Fix and improve auth_saml #342
Conversation
vincent-hatakeyama
changed the title
vincent-hatakeyama
force-pushed
the
topic/15.0/auth_saml
branch
2 times, most recently
from
50146d3 to
3819571
Compare
|
Thanks for your contrib! |
I have been asked to squash history in the past, I believe it is still the process at OCA.
All changes have been put in the readme/ directory too. |
vincent-hatakeyama
force-pushed
the
topic/15.0/auth_saml
branch
from
3819571 to
65b960a
Compare
Normally we ask to squash fixup commits or little changes on top of another. Here we have a lot of work squashed on a commit that says only " Fix the module by adding a transaction to commit the token " which is just a tiny piece of your work 😉 |
🚑 Fix the disallow password for users with SAML ids Added tests to ensure the feature works correctly. Admin user is also an exception from not having a password. In Odoo 15.0, this is the standard user to connect for administrative task, not the super user. ✨ Improve provider form and list views ✨⏩ port of 11.0 automatic redirection from 11.0 version. Use disable_autoredirect as a parameter query to disable automatic redirection (for example https://example.com/web/login?disable_autoredirect=) 💄 Add certificate file name fields to improve the UI 📝 Add required on several fields of the SAML provider; without them the server will crash and there is not enough information to make SAML work. ✨ Split signing to have finer control and be compatible with more IDP. 🔨 Integrate token into res.users.saml, removing auth_saml.token. No need for a separate table, and no more need to create lines in the table. 📝 Avoid server errors when user try metadata page without necessary parameters. 🚑 Replace method call from odoo.http.redirect_with_hash to request.redirect as the former does not exists in Odoo 15.0 anymore. 📚 Improved the module documentation 👕 pylint fixes and other fixes or minor changes
vincent-hatakeyama
force-pushed
the
topic/15.0/auth_saml
branch
from
65b960a to
7ee9295
Compare
gurneyalex
left a comment
gurneyalex
left a comment
There was a problem hiding this comment.
Tested with Azure Active Directory. Works well.
|
/ocabot merge minor |
|
Hey, thanks for contributing! Proceeding to merge this for you. |
|
@gurneyalex your merge command was aborted due to failed check(s), which you can inspect on this commit of 15.0-ocabot-merge-pr-342-by-gurneyalex-bump-minor. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
|
oca bot issue should be solved by #365 |
|
/ocabot merge minor |
|
What a great day to merge this nice PR. Let's do it! |
|
Congratulations, your PR was merged at 0f6f1d3. Thanks a lot for contributing to OCA. ❤️ |
Backport the refactoring of auth_saml done in OCA#342 🚑 Fix the module by adding a transaction to commit the token 🚑 Fix the disallow password for users with SAML ids Added tests to ensure the feature works correctly. Admin user is also an exception from not having a password. In Odoo 15.0, this is the standard user to connect for administrative task, not the super user. ✨ Improve provider form and list views ✨⏩ port of 11.0 automatic redirection from 11.0 version. Use disable_autoredirect as a parameter query to disable automatic redirection (for example https://example.com/web/login?disable_autoredirect=) 💄 Add certificate file name fields to improve the UI 📝 Add required on several fields of the SAML provider; without them the server will crash and there is not enough information to make SAML work. ✨ Split signing to have finer control and be compatible with more IDP. 🔨 Integrate token into res.users.saml, removing auth_saml.token. No need for a separate table, and no more need to create lines in the table. 📝 Avoid server errors when user try metadata page without necessary parameters. 🚑 Replace method call from odoo.http.redirect_with_hash to request.redirect as the former does not exists in Odoo 15.0 anymore. 📚 Improved the module documentation 👕 pylint fixes and other fixes or minor changes
Backport the refactoring of auth_saml done in OCA#342 🚑 Fix the module by adding a transaction to commit the token 🚑 Fix the disallow password for users with SAML ids Added tests to ensure the feature works correctly. Admin user is also an exception from not having a password. In Odoo 15.0, this is the standard user to connect for administrative task, not the super user. ✨ Improve provider form and list views ✨⏩ port of 11.0 automatic redirection from 11.0 version. Use disable_autoredirect as a parameter query to disable automatic redirection (for example https://example.com/web/login?disable_autoredirect=) 💄 Add certificate file name fields to improve the UI 📝 Add required on several fields of the SAML provider; without them the server will crash and there is not enough information to make SAML work. ✨ Split signing to have finer control and be compatible with more IDP. 🔨 Integrate token into res.users.saml, removing auth_saml.token. No need for a separate table, and no more need to create lines in the table. 📝 Avoid server errors when user try metadata page without necessary parameters. 🚑 Replace method call from odoo.http.redirect_with_hash to request.redirect as the former does not exists in Odoo 15.0 anymore. 📚 Improved the module documentation 👕 pylint fixes and other fixes or minor changes
|
Feedback on
This breaks auth_saml_environment module where the 2 key fields becomes non mandatory as we provide an alternative field with a path instead. Also seems #315 is not addressed correctly as the required key fields seem to be conditional depending on signature. |
Restore the usage of the form view by dropping the required condition on binary fields that are readonly. Those fields became mandatory after to OCA/server-auth#342 but the module auth_saml_environment offer to use paths as a alternative config option for binaries thus those fields can be empty.
|
The fields were changed to mandatory to avoid the issue, not to solve it. (I indicated for now but nobody proposed a fix since then). |
Restore the usage of the form view by dropping the required condition on binary fields that are readonly. Those fields became mandatory after to OCA/server-auth#342 but the module auth_saml_environment offer to use paths as a alternative config option for binaries thus those fields can be empty.
Restore the usage of the form view by dropping the required condition on binary fields that are readonly. Those fields became mandatory after OCA/server-auth#342 but the module auth_saml_environment offer to use paths as a alternative config option for binaries thus those fields can be empty.
Restore the usage of the form view by dropping the required condition on binary fields that are readonly. Those fields became mandatory after OCA/server-auth#342 but the module auth_saml_environment offer to use paths as a alternative config option for binaries thus those fields can be empty.
Syncing from upstream OCA/server-auth (14.0)
Restore the usage of the form view by dropping the required condition on binary fields that are readonly. Those fields became mandatory after OCA/server-auth#342 but the module auth_saml_environment offer to use paths as a alternative config option for binaries thus those fields can be empty.
7 participants
🚑 Fix the module by adding a transaction to commit the token
🚑 Fix the disallow password for users with SAML ids
Added tests to ensure the feature works correctly.
Admin user is also an exception from not having a password. In Odoo 15.0, this is the standard user to connect for administrative task, not the super user.
✨ Improve provider form and list views
✨⏩ port of 11.0 automatic redirection from 11.0 version. Use disable_autoredirect as a parameter query to disable automatic redirection (for example https://example.com/web/login?disable_autoredirect=)
💄 Add certificate file name fields to improve the UI
📝 Add required on several fields of the SAML provider; without them the server will crash and there is not enough information to make SAML work.
✨ Split signing to have finer control and be compatible with more IDP.
🔨 Integrate token into res.users.saml, removing auth_saml.token. No need for a separate table, and no more need to create lines in the table.
📝 Avoid server errors when user try metadata page without necessary parameters.
🚑 Replace method call from odoo.http.redirect_with_hash to request.redirect as the former does not exists in Odoo 15.0 anymore.
📚 Improved the module documentation
👕 pylint fixes and other fixes or minor changes
I’ve tested the module with a local keycloak. As I wrote in #337 (comment) the module did not work correctly, the base functionality was not working.
I’ve also fixed the allow password option and added tests.
My changes for the sign attribute on the provider comes from testing, where keycloak does not handle all the default signing, and from #321 indicating other IdP can have the same kind of needs.
I’ve ported the automatic redirection to a SAML provider from never merged #107
I’ve updated the README manually because it does not seem to be generated automatically. It might be missing some files in the readme directory, so I’ve added them. Same for the
static/description/index.htmlfile.#315 also affects this version, so I’ve made the certificates mandatory for now.