[16.0][ADD] hr_attendance_ip_check: IP-based attendance validation #189
Conversation
kongkea-aditi
changed the title
There was a problem hiding this comment.
There's already an open PR for v17 for a module(also having the same module name - hr_attendance_ip_check) in this repo itself (#168). It was opened quite a while ago - in June.The changes added through that PR does implement the same feature in a much better way. It takes into account CIDR for IP address checking and also it manages the whitelisted IPs in a much more efficient way.Also, the overall coding standards are better. So, it could have been simply backported to v16 rather than having the module developed through this PR.
hr_attendance_ip_check/__init__.py
Outdated
| @@ -0,0 +1,2 @@ | |||
| """IP-based Attendance Check module initialization.""" | |||
| import logging | ||
| from odoo import models, api, _ | ||
|
|
||
| _logger = logging.getLogger(__name__) |
| def create(self, vals_list): | ||
| """Override create to validate IP before creating attendance records.""" | ||
| if not vals_list: | ||
| return {'warning': _('No valid attendance records to create')} |
There was a problem hiding this comment.
No need to return warning for missing vals_list , it would be there. Anyways warning cannot be used here like this
| validation_result = employee._validate_ip_address('check_in') | ||
|
|
||
| if isinstance(validation_result, dict): | ||
| return validation_result |
There was a problem hiding this comment.
Warnings cannot be returned from create functions its only used for onchange functions. Instead of warnings throw ValidationError directly from _validate_ip_address function
|
|
||
| def write(self, vals): | ||
| """Override write to validate IP before updating attendance records.""" | ||
| if 'check_out' in vals: |
There was a problem hiding this comment.
Check for both check in and check out as check in time can also be modified later and accordingly pass check_in/check_out on line 31
| for attendance in self: | ||
| validation_result = attendance.employee_id._validate_ip_address('check_out') | ||
| if isinstance(validation_result, dict): | ||
| return validation_result |
There was a problem hiding this comment.
Similarly to create functions, warnings cannot be returned
|
|
||
| def _validate_ip_address(self, action_type='attendance'): | ||
| """Validate if current IP is allowed for attendance actions.""" | ||
| if not self.env['ir.config_parameter'].sudo().get_param( |
There was a problem hiding this comment.
Make use of const_eval(after importing it)
from odoo.tools.safe_eval import const_eval
const_eval(self.env['ir.config_parameter'].sudo().get_param('hr_attendance.ip_check_enabled', 'False')))
| _logger.error("Error getting IP address: %s", str(e), exc_info=True) | ||
| return None | ||
|
|
||
| def _validate_ip_address(self, action_type='attendance'): |
There was a problem hiding this comment.
return validation errors instead of warnings in this function
|
|
||
| # Validate IP first | ||
| ip_validation = self._validate_ip_address(action_type) | ||
| if isinstance(ip_validation, dict): |
There was a problem hiding this comment.
Validation error should be raised so no need to checking if its a dictionary
| config_parameter='hr_attendance.ip_check_enabled', | ||
| help="Enable IP address validation for attendance check-in/check-out" | ||
| ) | ||
| whitelist_ips = fields.Char( |
There was a problem hiding this comment.
Instead of having a comma separated list of IPs which would be very difficult to handle in long run, some other way should be used to handle the IPs.For ex: The allowed IPs can be stored on a per employee basis
- Renamed module to reflect architectural improvements - Inspired by PR OCA#168, uniquely adapted for work location-based architecture - Implement CIDR-based network ranges per work location - Add enterprise scalability with location-based IP management - Add per-location bypass and validation rules - Implement proper validation error handling - Add comprehensive test coverage - Improve security with proper access controls - Add multi-company support - Fix warning returns in create/write methods - Add detailed documentation
- Addressed issues related to pre-commit - Updated existing documentation to be more concise - Added roadmap planning multi-location setup and support for new odoo versions
…attendance ip address for better compatibility and scalability
…ired codecov coverage
…codecov requirements
- Refactor single test file to modular structure - Include more tests to increase coverage - Update documentation to indicate more test coverage needed - Add exception in coverage for common file shared by tests
- Restrict bypass ip check to only hr manager - Update and consolidate test hr attendance access accordingly - Mention test consolidation in documentation
- Update get remote ip check to be more robust
- Improve error handling and fallback logic for get remote ip method - Raise ValidationError for missing CIDR ranges and remove unnecessary exception in ip is allowed method - Refactor test hr attendance to isolate the ip mocking and add extra test cases to cover uncovered paths of other tests - Remove unneccesary coveragerc
|
@ByteMeAsap Thanks for your valuable feedback. This module was originally intended as a simple MVP for validating attendance based on a single location with a few IPs, but has been reworked to become more scalable and complete and is now designed for companies with multiple locations that leverages the Odoo work location architecture. This module now supports multiple CIDR ranges per location and includes comprehensive unit tests. I have credited the author of PR #68 and wouldn't mind adding the author of PR #68 as a contributor to this module shall it be required by the OCA. Kindly view my README for more details which include a roadmap to make this module more stable and final. It has been a great learning experience improving my Odoo skills through my first contribution to the OCA. |
| </record> | ||
|
|
||
| <!-- Employee bypass_ip_check field access --> | ||
| <record id="employee_bypass_ip_check_modify_rule" model="ir.rule"> |
| def create(self, vals_list): | ||
| """Validate IP before creating attendance records.""" | ||
| if not vals_list: | ||
| return super().create(vals_list) |
There was a problem hiding this comment.
No need to check for vals_list, it would be there
| "author": "Odoo Community Association (OCA)", | ||
| "website": "https://github.com/OCA/hr-attendance", | ||
| "license": "AGPL-3", | ||
| "depends": ["hr_attendance", "hr"], |
There was a problem hiding this comment.
Non need to hr module as dependency as hr_attendance dependency would take care of that
| def create(self, vals_list): | ||
| """Validate IP before creating attendance records.""" | ||
| if not vals_list: | ||
| return super().create(vals_list) |
There was a problem hiding this comment.
vals_list would be there, no need to put a check for it
| return super().create(vals_list) | ||
|
|
||
| for vals in vals_list: | ||
| employee = self.env["hr.employee"].browse(vals.get("employee_id")) |
There was a problem hiding this comment.
You can store hr.employee object in a variable and use it in the loop
| ) | ||
| ) | ||
|
|
||
| def check_ip_allowed(self, ip_addr): |
| _description = "Work Location CIDR Network" | ||
| _order = "sequence, id" | ||
|
|
||
| sequence = fields.Integer(default=10, index=True) |
|
|
||
| ip_check_enabled = fields.Boolean( | ||
| string="Enable IP Attendance Check for Work Locations", | ||
| config_parameter="hr_attendance.ip_check_enabled", |
| </record> | ||
|
|
||
| <!-- Employee bypass_ip_check field access --> | ||
| <record id="employee_bypass_ip_check_modify_rule" model="ir.rule"> |
| groups="hr.group_hr_manager" | ||
| help="When enabled, this employee can check in/out from any IP address regardless of work location settings" | ||
| /> | ||
| <field name="work_location_id" /> |
There was a problem hiding this comment.
This field is already present in the base employee form
…work location when global setting disabled
…st check in hr attendance
…hen create attendance
…cation check when create attendance
…cation check when update attendance
… location ip func
…odule name for ip check enabled config
…conditions to be more efficient
…with logging instead when ip address throw value error
…d improve error handling
…al from check ip allowed
…eld from hr work location cidr
…ation id from employee view
…elated to unused check ip allowed & validate location ip) and update according to get remote ip change
|
Thank @ByteMeAsap, I have updated based on latest feedbacks kindly take another look. |
[16.0][ADD] hr_attendance_ip_check: IP-based attendance validation
This module extends HR attendance to validate check-in/check-out based on IP address.
Features:
Test scenario: