-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[MIG] auth_brute_force: Migration to 10.0 #877
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
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.
Tried also on runbot with a non correct behavior:
- Login n times until I reach the limit.
- On last attempt, it gets me to an incorrect "Internal Server Error", and no banned remote is written.
- Then, I'm able to login again with correct credentials.
auth_brute_force/README.rst
Outdated
|
|
||
| * Authentication failed from remote '127.0.0.1'. Login tried : 'admin'. | ||
| Attempt 1 / 10. | ||
| Attempt 1 / 10. |
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.
Why do you remove indentation? This provokes an incorrect RST syntax. It was correct as it was.
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.
Seem like my editor tried somehow to re-indent it...
auth_brute_force/README.rst
Outdated
| Known issues / Roadmap | ||
| ====================== | ||
|
|
||
| The ID used to identify a remote request is the IP provided in the request |
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.
Put * and indent the rest of the lines.
auth_brute_force/README.rst
Outdated
| of the user can be wrong, and mainly in the following cases: | ||
|
|
||
| * 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; |
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.
Indent the line
auth_brute_force/README.rst
Outdated
| * 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.
Indent the line
auth_brute_force/__manifest__.py
Outdated
| 'category': 'Tools', | ||
| 'summary': "Tracks Authentication Attempts and Prevents Brute-force" | ||
| " Attacks module", | ||
| 'author': "GRAP,Odoo Community Association (OCA)", |
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.
Add Tecnativa here
| <openerp> | ||
| <!-- Copyright 2015 GRAP -Sylvain LE GAL | ||
| License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). --> | ||
| <odoo> |
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.
Put directly <odoo noupdate="1">
| "Authentication failed from remote '%s'. " | ||
| "The remote has been banned. Login tried : '%s'" | ||
| "." % (remote, request.params['login'])) | ||
| banned_remote_obj.create(cursor, SUPERUSER_ID, { |
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 haven't converted this to new API (banned_remote_obj.sudo().create({ ... }))
| @@ -0,0 +1,77 @@ | |||
| # -*- coding: utf-8 -*- | |||
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.
This file is usually named main.py.
| attempt_obj = env['res.authentication.attempt'] | ||
| banned_remote_obj = env['res.banned.remote'] | ||
| # Get Settings | ||
| max_attempts_qty = int(config_obj.search_read( |
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.
Use config_obj.get_param('auth_brute_force.max_attempt_qty') instead of search_read.
| request.session.db, request.params['login'], | ||
| request.params['password']) | ||
| # Log attempt | ||
| cursor.commit() |
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.
There is a commit just after the next instruction, is this commit really needed ?
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.
Yes. It seems that it's not neccesary.
| <!-- Copyright 2015 GRAP -Sylvain LE GAL | ||
| License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). --> | ||
| <odoo> | ||
| <data noupdate="1"> |
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.
Remove the data node and include the noupdate attribute in the odoo node.
| string='Description', compute='_compute_description', store=True) | ||
|
|
||
| ban_date = fields.Datetime( | ||
| string='Ban Date', required=True, default=_default_ban_date) |
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.
default=fields.Datetime.now.
| def _compute_attempt_ids(self): | ||
| for item in self: | ||
| attempt_obj = self.env['res.authentication.attempt'] | ||
| item.attempt_ids = attempt_obj.search_last_failed(item.remote).ids |
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 can remove the trailing .ids, Odoo allows you to assign a recordset.
|
@pedrobaeza @sylvain-garancher I coded your suggestions and tested functionally in local. |
5ac1662 to
18f4dee
Compare
18f4dee to
e91bdfb
Compare
Merge 15.0 into 15.0.project_MI_465
Tracks Authentication Attempts and Prevents Brute-force Attacks
This module registers each request done by users trying to authenticate into
Odoo. If the authentication fails, a counter is increased for the given remote
IP. After a defined number of attempts, Odoo will ban the remote IP and
ignore new requests.
cc @Tecnativa