-
-
Notifications
You must be signed in to change notification settings - Fork 492
[14.0][MIG] auth_saml #268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
6132212 to
bca1241
Compare
|
FTR we created this module to automatise the configuration of this module by environment: https://github.com/OCA/server-env/pull/74/files |
|
@theangryangel I propesed you a PR to extend the configurations on signatures here: |
|
@theangryangel beware my change is broken on the signatures I missed some |
|
@theangryangel feel free to remove my commit as it is unstable. |
|
This PR has the |
|
@pedrobaeza this PR seems ready to be merged ! :) |
pedrobaeza
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution.
Please preserve commit history following technical method explained in https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-14.0.
Please squash migration commits as well.
1358c57 to
74ba896
Compare
74ba896 to
7218cd1
Compare
7218cd1 to
ed2f30f
Compare
|
@theangryangel Hey, what's the situation with this PR, any estimation when this will merged? Does the module make it possible for users to signup automatically if no user with matching attribute exist and Free sign up is enabled? |
@savijoki no idea, I don't have the ability to merge into this repo. Ignoring the pre-commit issue (which is a 2 minute fix that I'll sort today), this is in the same state that the 13.0 was. You can see how long that took here. If it helps we are running it in production direct from the branch, it should be relatively safe to do so.
Currently no. In terms of user setup both the 13.0 and 14.0 ports follow how 12.0 was implemented. It could certainly be added, but in my experience you'd need someone to go and fiddle with the user account to set the correct permissions for a new user anyway. Very few of our customers all run with the same group memberships 🤷 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional review. 👍 works great on 14.0 (Enterprise)
Possible addition on this module: Allow to auto create users in Odoo if not present
| "website": "https://github.com/OCA/server-auth", | ||
| "license": "AGPL-3", | ||
| "depends": ["base_setup"], | ||
| "external_dependencies": {"python": ["pysaml2"]}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it is saml2 that is used instead of pysaml2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Hi @pedrobaeza this PR is now ready to be merged. Thanks |
The following line of code for 11.0: - https://github.com/odoo/odoo/blob/52d6f0e3ee90874fc93fec9cdff74ec71d3b991f/addons/auth_oauth/controllers/main.py#L69 is assigning the key "auth_link" for "list_providers" method. The following template is expecting this key: - https://github.com/odoo/odoo/blob/52d6f0e3ee90874fc93fec9cdff74ec71d3b991f/addons/auth_oauth/views/auth_oauth_templates.xml#L5 So, it raise a KeyError compiling "template_auth_oauth_providers_N" This change is fixing adding that expected key in order to avoid this KeyError
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: server-auth-11.0/server-auth-11.0-auth_saml Translate-URL: https://translation.odoo-community.org/projects/server-auth-11-0/server-auth-11-0-auth_saml/
[FIX] dependencies
add requirement on lasso
- Default behavior is now to allow password and SAML together. Otherwise, users could keep getting their passwords removed without warning. - General cleanup. - Remove relations to field `password_crypt` because in v12 the `password` field is always encrypted instead. Co-Authored-By: Alexandre Díaz <[email protected]>
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: server-auth-12.0/server-auth-12.0-auth_saml Translate-URL: https://translation.odoo-community.org/projects/server-auth-12-0/server-auth-12-0-auth_saml/
ed2f30f to
9f60a1a
Compare
Rebased, pre-commit manually run and pushed. Sorry, I thought I had done that weeks ago for some reason. |
|
Hi @pedrobaeza it has been rebased. I hope this time is the right one. |
|
/ocabot merge nobump |
|
On my way to merge this fine PR! |
|
Congratulations, your PR was merged at 0af0920. Thanks a lot for contributing to OCA. ❤️ |
|
thanks @pedrobaeza ! |
|
FTR tests for this module are broken ATM #330 |
Syncing from upstream OCA/server-auth (13.0)
Forward port of #214.
We've got this running on our production install so I'm reasonably happy it's OK. Again happy to learn/take comments, etc. :)