-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
8.0 add auth track and prevent brut force #262
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
8.0 add auth track and prevent brut force #262
Conversation
|
Very interesting module. I think the name can be shorten to auth_brute_force, because we don't need to put the full title on the name 😉 |
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.
s/Brut Force/Brut-force attacks
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.
Does that call delay the login? It should be then put on Known issues, or prepare an asynchronous call for that
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.
The call is done when the user is banned. So it doesn't delay the login...
|
Good icon! Do you have the SVG version? Can you remove blank background from it and let it transparent? |
|
Please put also icon reference on credits (ensuring it can be used - CC license?) |
|
Sorry for bothering you again. In last commit you have changed references to |
|
hi @pedrobaeza ! Thanks for your review !
I changed the rest according to your other comments. |
auth_brute_force/__openerp__.py
Outdated
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.
Suggestion:
Tracks Authentication Attempts and Prevents Brute-force Attacks
or
Authentication Tracking and Brute-force Attacks Prevention
=> 2 verbs or 2 nouns. I prefer the first one.
…ithub.com/grap/server-tools into 8.0_ADD_auth_track_and_prevent_brut_force
|
One minor remark: The rest seems 👍 (code review) |
|
Thanks for your review, @pedrobaeza. |
|
👍 |
…_force [ADD] auth track and prevent brut force
|
Hi @legalsylvain , I would like to ash about this module after reading README. Does it notify to any user in Odoo to take care about? (notify -> email). Thanks! |
| * if the Odoo server is behind an Apache / NGinx proxy without redirection, | ||
| all the request will be have the value '127.0.0.1' for the REMOTE_ADDR key; | ||
| * If some users are behind the same Internet Service Provider, if a user is | ||
| banned, all the other users will be banned too; |
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.
But this is expected to be the usual case, isn't it?
I'm a little out of step here, be shouldn't we also look into X-Forwarded-For?
| model_id: model_res_authentication_attempt | ||
| name: Authentication Attempt All Users | ||
| perm_read: true | ||
|
|
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 think it's a kind of privacy flaw to allow all users to read res.authentication.attempt. And it's not needed to give that right to all users.
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.
You're probably right. (the same for model_res_banned_remote I guess)
feel free to propose a PR, I'll review it quickly. (you can too replace by xml, as yml is now deprecated)
Syncing from upstream OCA/server-tools (10.0)
Add a module to limit brut force attack on authentication.
Define an ir.config_parameter that mentions max attempts.
Once this maximum reached, the remote user will be blocked. (by IP).
See readme file for the whole description.
Thanks for your review.