-
Notifications
You must be signed in to change notification settings - Fork 2
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
[9.0] patch catch ldap error #5
base: 9.0
Are you sure you want to change the base?
Conversation
@robinkeunen ce n'est pas mieux d'aussi la proposer à l'OCB ? |
Non, c’est trop brutal: j’attrape une LDAPError dans le point d’entrée de l’an route /web/login...
Robin Keunen
Coop IT Easy
[email protected]
+32 488 86 57 40
…On 30 Mar 2020, 12:39 +0200, Houssine BAKKALI ***@***.***>, wrote:
@robinkeunen ce n'est pas mieux d'aussi la proposer à l'OCB ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
uid = request.session.authenticate(request.session.db, request.params['login'], request.params['password']) | ||
try: | ||
uid = request.session.authenticate(request.session.db, request.params['login'], request.params['password']) | ||
except LDAPError: |
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.
ah oui, je n'avais pas vu que c'était dans le module standard, je comprends pourquoi tu dis que c'est crade...
et quoi overrider la fonction authenticate de l'OpenERPSession n'est pas possible ? https://github.com/OCA/OCB/blob/9.0/openerp/http.py#L1111
ça me parait plus propre de le faire dans un module à part en auto_install lié au module LDAP ou dans l'inclure dans module LDAP lui-même ce qui a le plus de sens.
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.
@robinkeunen du coup ce que je propose ici à plus de sens.
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.
@houssine78 Yes, ça a du sens, mais si j'en viens à de telle nécessités c'est parce que les autres options ne marchent pas, l'exception me file entre les doigts. @vvrossem s'est aussi cassé les dents.
(j'ai pas essayé de faire juste un module d'overide, juste de patcher auth_users
et auth_ldap
)
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.
Proposition suite à la lecture des commentaires ci-dessus et des notes de la tâche : vu que ce n'est pas possible de tester sur la test, voyons si le patch passe sur la prod et overridons authenticate
le cas échéant
ok du coup pour ce patch en attendant mieux |
eada71c
to
0e33916
Compare
C'est un peu moche mais ça fait le taff