[19.0][MIG] auth_saml: Migration to 19.0 - #916
vincent-hatakeyama wants to merge 80 commits into
Conversation
ea3da06 to
57bc71b
Compare
dadd3b2 to
02262c0
Compare
02262c0 to
43b377b
Compare
|
Rebased and added fix #937 |
|
/ocabot migration auth_saml |
|
Sorry @vincent-hatakeyama you are not allowed to mark the addon to be migrated. To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons. If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the |
43b377b to
a489d35
Compare
|
/ocabot merge nobump |
|
Sorry @vincent-hatakeyama you are not allowed to merge. To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons. If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the |
|
As before, I can’t merge this because of the change to If anyone is using this PR, please approve it so I ask project maintainers to merge it. |
Hi @vincent-hatakeyama , I tried your latest code and the part not sending out the security notification that password has changed does not seem to work. I still get an email when creating a user with SAML configured that password has changed. So he receives a welcome notification (which we can adjust), but also immediately receives an email that the password was changed. If there is anything I can do to help debugging this, please let me know. It seems this PR was cloesed? So what needs to happen to get this merged as we use this module. |
|
Hello everyone. I am also happy to help here. |
I just checked and I do not reproduce this ; the new user only get the welcome notification.
I need more detail on what is done on a newly created database to reproduce.
This PR is not closed. It needs reviews to get merged. Even when not merged, it is usable ; one of our customer uses it in production and another will also do at the end of the month. |
The code coverage is not a blocking issue but if you have time to improve it, it’s welcome. Reviews with approval is what is really needed to get this PR merged. |
58c4f33 to
f756454
Compare
|
Hello, I also have a customer that is using it in production, we just did the setup today! Can we merge? Hugo |
|
@OCA/tools-maintainers Hi. Can someone merge this? There is enough positive reviews (in comments), and the only reason I can’t merge (despite being a maintainer) is because it touches the requirements file outside the addon directory. |
|
/ocabot merge nobump /ocabot migration auth_saml |
Fix logic of SELECT FOR UDPDATE to only lock records whose metadata will be updated
When using mapping, not writing the value systematically avoids getting security mail on login/email changes when there is no change. Also use SQL for blanking passwords avoids the security update mails.
Currently translated at 100.0% (93 of 93 strings) Translation: server-auth-18.0/server-auth-18.0-auth_saml Translate-URL: https://translation.odoo-community.org/projects/server-auth-18-0/server-auth-18-0-auth_saml/it/
f756454 to
9663fed
Compare
|
This is soooo close to the finish line! |
274a102 to
c9caa2d
Compare
0dcee26 to
4ab742d
Compare
|
@thomaspaulb Rebased and fixed/improved. It should now merge without issue. Thanks! |
- 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 - add an opton to reactivate user when finding an user and creation is enabled
4ab742d to
34018c5
Compare
|
/ocabot merge nobump |
|
Hey, thanks for contributing! Proceeding to merge this for you. |
|
@thomaspaulb your merge command was aborted due to failed check(s), which you can inspect on this commit of 19.0-ocabot-merge-pr-916-by-thomaspaulb-bump-nobump. 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. |
|
@vincent-hatakeyama Still a test failure, could you take a look? |
20 participants
There is already an existing PR that missed some needed changes. I also do not manage to log in with a local keycloak.
I’m currently facing the same issue with my PR (that’s why it is in draft).Fixed by looking at auth_oauth to find the issue after some digging.