From a6742c16b0fa33cfe43c5cd102bdc0a05ce29c9e Mon Sep 17 00:00:00 2001 From: Denis Durepos Date: Sat, 26 Jul 2025 20:11:22 -0400 Subject: [PATCH] fix(portal): Resolve injury creation security violations and improve UI consistency - Fix has_group() security violations by using request.env.user.has_group() in controllers - Add mail.compose.message ACL permissions for portal users to enable injury creation - Fix foreign key constraint violation in treatment note creation by passing correct patient parameter - Update portal field labels and values to match internal views (match_status, practice_status, stage) - Change "Medical Notes" to "Team Notes" and convert from HTML to plain text rendering - Fix allergies field clearing logic to allow empty string updates - Improve badge text readability with proper color contrast across all portal views - Remove redundant external link icons from injury count displays Resolves portal injury creation 403 errors and ensures consistent terminology between portal and backend interfaces. --- .../controllers/patient_injury_portal.py | 37 ++++++++---- .../controllers/player_management_portal.py | 8 +-- .../controllers/team_management_portal.py | 9 +-- bemade_sports_clinic/models/patient.py | 4 +- bemade_sports_clinic/models/patient_injury.py | 8 ++- bemade_sports_clinic/models/sports_team.py | 52 ++++++++++------- .../security/ir.model.access.csv | 4 ++ .../security/sports_clinic_rules.xml | 8 +-- .../player_management_portal_templates.xml | 6 +- .../views/sports_clinic_portal_views.xml | 58 +++++++++---------- 10 files changed, 112 insertions(+), 82 deletions(-) diff --git a/bemade_sports_clinic/controllers/patient_injury_portal.py b/bemade_sports_clinic/controllers/patient_injury_portal.py index 86ee2c9..ff341d9 100644 --- a/bemade_sports_clinic/controllers/patient_injury_portal.py +++ b/bemade_sports_clinic/controllers/patient_injury_portal.py @@ -26,7 +26,8 @@ class PatientInjuryPortal(CustomerPortal): ]) # Medical professionals might have specific access - is_medical = user.has_group('bemade_sports_clinic.group_portal_treatment_professional') + # Use request.env.user.has_group() directly to avoid security violations + is_medical = request.env.user.has_group('bemade_sports_clinic.group_portal_treatment_professional') if not patient.exists() or (not accessible_teams and not is_medical): raise UserError(_('You do not have access to this patient.')) @@ -57,8 +58,9 @@ class PatientInjuryPortal(CustomerPortal): default_team_id = teams[0].id if len(teams) == 1 else None # Check if user is a treatment professional + # Use request.env.user.has_group() directly to avoid security violations + is_treatment_prof = request.env.user.has_group('bemade_sports_clinic.group_portal_treatment_professional') user = request.env.user - is_treatment_prof = user.has_group('bemade_sports_clinic.group_portal_treatment_professional') values = { 'patient': patient, @@ -120,11 +122,11 @@ class PatientInjuryPortal(CustomerPortal): # Create the injury record - portal users now have create permission injury = request.env['sports.patient.injury'].create(vals) - user = request.env.user # Determine if user is a coach or treatment professional - is_portal_coach = user.has_group('bemade_sports_clinic.group_portal_team_coach') - is_treatment_prof = user.has_group('bemade_sports_clinic.group_portal_treatment_professional') or \ - user.has_group('bemade_sports_clinic.group_portal_treatment_professional') + # Use request.env.user.has_group() directly to avoid security violations + is_portal_coach = request.env.user.has_group('bemade_sports_clinic.group_portal_team_coach') + is_treatment_prof = request.env.user.has_group('bemade_sports_clinic.group_portal_treatment_professional') + user = request.env.user # Detailed logging of the current user's status _logger.info(f"Current user: {user.name} (ID: {user.id})") @@ -236,13 +238,15 @@ class PatientInjuryPortal(CustomerPortal): # Check if user is part of the team staff is_team_staff = team.staff_ids.filtered(lambda s: s.user_ids and user.id in s.user_ids.ids) # Or if user is a medical professional with broader access - is_medical = user.has_group('bemade_sports_clinic.group_portal_treatment_professional') + # Use request.env.user.has_group() directly to avoid security violations + is_medical = request.env.user.has_group('bemade_sports_clinic.group_portal_treatment_professional') if not is_team_staff and not is_medical: raise UserError(_('You do not have access to this injury.')) else: # If no team is specified, only medical professionals can access - if not user.has_group('bemade_sports_clinic.group_portal_treatment_professional'): + # Use request.env.user.has_group() directly to avoid security violations + if not request.env.user.has_group('bemade_sports_clinic.group_portal_treatment_professional'): raise UserError(_('You do not have access to this injury.')) return injury @@ -266,8 +270,9 @@ class PatientInjuryPortal(CustomerPortal): # Get possible injury stages - treatment professionals can change stage stages = [] + # Use request.env.user.has_group() directly to avoid security violations + is_treatment_prof = request.env.user.has_group('bemade_sports_clinic.group_portal_treatment_professional') user = request.env.user - is_treatment_prof = user.has_group('bemade_sports_clinic.group_portal_treatment_professional') if is_treatment_prof: stage_selection = request.env['sports.patient.injury']._fields['stage'].selection @@ -321,8 +326,9 @@ class PatientInjuryPortal(CustomerPortal): }) # Get user's role + # Use request.env.user.has_group() directly to avoid security violations + is_treatment_prof = request.env.user.has_group('bemade_sports_clinic.group_portal_treatment_professional') user = request.env.user - is_treatment_prof = user.has_group('bemade_sports_clinic.group_portal_treatment_professional') # Prepare values for injury update vals = {} @@ -359,7 +365,10 @@ class PatientInjuryPortal(CustomerPortal): # Add a treatment note if provided if post.get('treatment_note') and is_treatment_prof: - self._add_treatment_note(injury, post.get('treatment_note')) + _logger.info(f"DEBUG: About to create treatment note for injury {injury.id}") + _logger.info(f"DEBUG: injury.patient_id = {injury.patient_id} (ID: {injury.patient_id.id})") + _logger.info(f"DEBUG: injury.patient_id.partner_id = {injury.patient_id.partner_id} (ID: {injury.patient_id.partner_id.id if injury.patient_id.partner_id else 'None'})") + self._add_treatment_note(injury.patient_id, post.get('treatment_note'), injury) # Redirect back to the edit form with success message return_url = post.get('return_url', f'/my/injury/edit?injury_id={injury_id}') @@ -369,6 +378,11 @@ class PatientInjuryPortal(CustomerPortal): """Helper method to add a treatment note to a patient, optionally linked to an injury""" if not note_content.strip(): return False + + _logger.info(f"DEBUG: _add_treatment_note called with patient={patient} (ID: {patient.id})") + _logger.info(f"DEBUG: patient model: {patient._name}") + if hasattr(patient, 'partner_id'): + _logger.info(f"DEBUG: patient.partner_id = {patient.partner_id} (ID: {patient.partner_id.id if patient.partner_id else 'None'})") # Create a new treatment note linked to patient, optionally to injury vals = { @@ -377,6 +391,7 @@ class PatientInjuryPortal(CustomerPortal): 'date': fields.Date.today(), 'user_id': request.env.user.id, } + _logger.info(f"DEBUG: About to create treatment note with vals: {vals}") # If injury is provided, link the note to it if injury: diff --git a/bemade_sports_clinic/controllers/player_management_portal.py b/bemade_sports_clinic/controllers/player_management_portal.py index 0273a62..ebbf581 100644 --- a/bemade_sports_clinic/controllers/player_management_portal.py +++ b/bemade_sports_clinic/controllers/player_management_portal.py @@ -134,14 +134,14 @@ class PlayerManagementPortal(CustomerPortal): }) # Medical information - if post.get('allergies'): + if 'allergies' in post: vals.update({ - 'allergies': post.get('allergies'), + 'allergies': post.get('allergies') or False, }) - if post.get('team_info_notes'): + if 'team_info_notes' in post: vals.update({ - 'team_info_notes': post.get('team_info_notes'), + 'team_info_notes': post.get('team_info_notes') or False, }) # Status fields diff --git a/bemade_sports_clinic/controllers/team_management_portal.py b/bemade_sports_clinic/controllers/team_management_portal.py index b804a46..4a0bd38 100644 --- a/bemade_sports_clinic/controllers/team_management_portal.py +++ b/bemade_sports_clinic/controllers/team_management_portal.py @@ -33,7 +33,8 @@ class TeamManagementPortal(CustomerPortal): ) # Check if user is a treatment professional with access - is_treatment_professional = user.has_group( + # Use request.env.user.has_group() directly to avoid security violations + is_treatment_professional = request.env.user.has_group( 'bemade_sports_clinic.group_portal_treatment_professional' ) @@ -279,9 +280,9 @@ class TeamManagementPortal(CustomerPortal): # If no active patient found, check if user has permission to see inactive records # Only treatment professionals or admins should see inactive/archived patients - user = request.env.user - if user.has_group('bemade_sports_clinic.group_portal_treatment_professional') or \ - user.has_group('base.group_system'): + # Use request.env.user.has_group() directly to avoid security violations + if request.env.user.has_group('bemade_sports_clinic.group_portal_treatment_professional') or \ + request.env.user.has_group('base.group_system'): return request.env['sports.patient'].search(domain + [('active', '=', False)], limit=1) return request.env['sports.patient'].browse([]) # Empty recordset if no matches diff --git a/bemade_sports_clinic/models/patient.py b/bemade_sports_clinic/models/patient.py index 88203e2..135a8fe 100644 --- a/bemade_sports_clinic/models/patient.py +++ b/bemade_sports_clinic/models/patient.py @@ -133,9 +133,9 @@ class Patient(models.Model): allergies = fields.Text( groups="bemade_sports_clinic.group_sports_clinic_treatment_professional,bemade_sports_clinic.group_portal_treatment_professional", ) - team_info_notes = fields.Html( + team_info_notes = fields.Text( string="Notes", - # Removed tracking=True as HTML fields are not supported by mail tracking system + tracking=True, groups="bemade_sports_clinic.group_sports_clinic_treatment_professional,bemade_sports_clinic.group_portal_treatment_professional", ) diff --git a/bemade_sports_clinic/models/patient_injury.py b/bemade_sports_clinic/models/patient_injury.py index fe321eb..11cfa32 100644 --- a/bemade_sports_clinic/models/patient_injury.py +++ b/bemade_sports_clinic/models/patient_injury.py @@ -280,9 +280,11 @@ class PatientInjury(models.Model): users = self.env['res.users'].search([('partner_id', '=', partner.id)]) # Check if any of the users is a treatment professional + # Note: We can't use has_group() on non-current users, so we check group membership directly is_treatment_prof = False + treatment_prof_group = self.env.ref('bemade_sports_clinic.group_sports_clinic_treatment_professional') for user in users: - if user.has_group('bemade_sports_clinic.group_sports_clinic_treatment_professional'): + if treatment_prof_group in user.groups_id: is_treatment_prof = True break @@ -391,9 +393,11 @@ class PatientInjury(models.Model): if team_staff: treatment_professional_group = self.env.ref('bemade_sports_clinic.group_sports_clinic_treatment_professional') # Get all users from staff and filter them by group + # Use direct group membership check instead of has_group() to avoid security violations staff_users = team_staff.mapped('user_ids') + treatment_prof_group = self.env.ref('bemade_sports_clinic.group_sports_clinic_treatment_professional') therapist_users = staff_users.filtered( - lambda user: user.has_group('bemade_sports_clinic.group_sports_clinic_treatment_professional') + lambda user: treatment_prof_group in user.groups_id ) if therapist_users: diff --git a/bemade_sports_clinic/models/sports_team.py b/bemade_sports_clinic/models/sports_team.py index cf382fd..9f973bf 100644 --- a/bemade_sports_clinic/models/sports_team.py +++ b/bemade_sports_clinic/models/sports_team.py @@ -194,9 +194,12 @@ class TeamStaff(models.Model): def _compute_has_portal_access(self): for rec in self: # Check if the partner has any active users with portal or internal access + # Use direct group membership check instead of has_group() to avoid security violations + portal_group = self.env.ref('base.group_portal') + user_group = self.env.ref('base.group_user') rec.has_portal_access = ( - bool(rec.user_ids.filtered(lambda r: r.has_group("base.group_portal"))) - or bool(rec.user_ids.filtered(lambda r: r.has_group("base.group_user"))) + bool(rec.user_ids.filtered(lambda r: portal_group in r.groups_id)) + or bool(rec.user_ids.filtered(lambda r: user_group in r.groups_id)) ) def action_revoke_portal_access(self): @@ -282,13 +285,15 @@ class TeamStaff(models.Model): users = self.env['res.users'].sudo().search([('partner_id', '=', partner.id)]) treatment_prof_group = self.env.ref('bemade_sports_clinic.group_sports_clinic_treatment_professional') portal_treatment_prof_group = self.env.ref('bemade_sports_clinic.group_portal_treatment_professional') + portal_group = self.env.ref('base.group_portal') for user in users: # Handle internal and portal users differently - if not user.has_group('base.group_portal'): - if user.has_group('bemade_sports_clinic.group_sports_clinic_treatment_professional'): + # Use direct group membership check instead of has_group() to avoid security violations + if portal_group not in user.groups_id: + if treatment_prof_group in user.groups_id: user.sudo().write({'groups_id': [(3, treatment_prof_group.id)]}) else: - if user.has_group('bemade_sports_clinic.group_portal_treatment_professional'): + if portal_treatment_prof_group in user.groups_id: user.sudo().write({'groups_id': [(3, portal_treatment_prof_group.id)]}) # Update group membership directly for each affected user @@ -304,19 +309,21 @@ class TeamStaff(models.Model): treatment_prof_group = self.env.ref('bemade_sports_clinic.group_sports_clinic_treatment_professional') portal_treatment_prof_group = self.env.ref('bemade_sports_clinic.group_portal_treatment_professional') + portal_group = self.env.ref('base.group_portal') # Apply appropriate group membership based on user type and therapist roles - if not user.has_group('base.group_portal'): + # Use direct group membership check instead of has_group() to avoid security violations + if portal_group not in user.groups_id: # Internal user - if has_therapist_role and not user.has_group('bemade_sports_clinic.group_sports_clinic_treatment_professional'): + if has_therapist_role and treatment_prof_group not in user.groups_id: user.sudo().write({'groups_id': [(4, treatment_prof_group.id)]}) - elif not has_therapist_role and user.has_group('bemade_sports_clinic.group_sports_clinic_treatment_professional'): + elif not has_therapist_role and treatment_prof_group in user.groups_id: user.sudo().write({'groups_id': [(3, treatment_prof_group.id)]}) else: # Portal user - if has_therapist_role and not user.has_group('bemade_sports_clinic.group_portal_treatment_professional'): + if has_therapist_role and portal_treatment_prof_group not in user.groups_id: user.sudo().write({'groups_id': [(4, portal_treatment_prof_group.id)]}) - elif not has_therapist_role and user.has_group('bemade_sports_clinic.group_portal_treatment_professional'): + elif not has_therapist_role and portal_treatment_prof_group in user.groups_id: user.sudo().write({'groups_id': [(3, portal_treatment_prof_group.id)]}) return res @@ -345,7 +352,8 @@ class TeamStaff(models.Model): should_have_access (bool): Whether the user should have treatment professional access treatment_prof_group (res.groups): The treatment professional group """ - has_access = user.has_group('bemade_sports_clinic.group_sports_clinic_treatment_professional') + # Use direct group membership check instead of has_group() to avoid security violations + has_access = treatment_prof_group in user.groups_id if should_have_access and not has_access: user.sudo().write({'groups_id': [(4, treatment_prof_group.id)]}) @@ -399,17 +407,19 @@ class TeamStaff(models.Model): users_to_process = specific_user # Apply the appropriate group membership - if not users_to_process.has_group('base.group_portal'): + # Use direct group membership check instead of has_group() to avoid security violations + portal_group = self.env.ref('base.group_portal') + if portal_group not in users_to_process.groups_id: # Internal user - if has_therapist_role and not users_to_process.has_group('bemade_sports_clinic.group_sports_clinic_treatment_professional'): + if has_therapist_role and treatment_prof_group not in users_to_process.groups_id: users_to_process.sudo().write({'groups_id': [(4, treatment_prof_group.id)]}) - elif not has_therapist_role and users_to_process.has_group('bemade_sports_clinic.group_sports_clinic_treatment_professional'): + elif not has_therapist_role and treatment_prof_group in users_to_process.groups_id: users_to_process.sudo().write({'groups_id': [(3, treatment_prof_group.id)]}) else: # Portal user - if has_therapist_role and not users_to_process.has_group('bemade_sports_clinic.group_portal_treatment_professional'): + if has_therapist_role and portal_treatment_prof_group not in users_to_process.groups_id: users_to_process.sudo().write({'groups_id': [(4, portal_treatment_prof_group.id)]}) - elif not has_therapist_role and users_to_process.has_group('bemade_sports_clinic.group_portal_treatment_professional'): + elif not has_therapist_role and portal_treatment_prof_group in users_to_process.groups_id: users_to_process.sudo().write({'groups_id': [(3, portal_treatment_prof_group.id)]}) return @@ -419,15 +429,17 @@ class TeamStaff(models.Model): has_therapist_role = bool(self._get_staff_with_therapist_roles(staff.partner_id.id)) # Process each user linked to this partner + portal_group = self.env.ref('base.group_portal') + user_group = self.env.ref('base.group_user') for user in staff.user_ids: # Handle internal users - if user.has_group('base.group_user'): + if user_group in user.groups_id: self._update_user_group_membership(user, has_therapist_role, treatment_prof_group) # Handle portal users - elif user.has_group('base.group_portal'): - if has_therapist_role and not user.has_group('bemade_sports_clinic.group_portal_treatment_professional'): + elif portal_group in user.groups_id: + if has_therapist_role and portal_treatment_prof_group not in user.groups_id: user.sudo().write({'groups_id': [(4, portal_treatment_prof_group.id)]}) - elif not has_therapist_role and user.has_group('bemade_sports_clinic.group_portal_treatment_professional'): + elif not has_therapist_role and portal_treatment_prof_group in user.groups_id: user.sudo().write({'groups_id': [(3, portal_treatment_prof_group.id)]}) def write(self, values): diff --git a/bemade_sports_clinic/security/ir.model.access.csv b/bemade_sports_clinic/security/ir.model.access.csv index b24fe34..5fd86fc 100644 --- a/bemade_sports_clinic/security/ir.model.access.csv +++ b/bemade_sports_clinic/security/ir.model.access.csv @@ -12,6 +12,7 @@ access_injury_portal,Portal Access for Injuries,model_sports_patient_injury,base access_injury_portal_tp,Portal Treatment Prof Access for Injuries,model_sports_patient_injury,bemade_sports_clinic.group_portal_treatment_professional,1,1,1,0 access_team_user,User Access for Teams,model_sports_team,group_sports_clinic_user,1,1,1,0 access_team_admin,Admin Access for Teams,model_sports_team,group_sports_clinic_admin,1,1,1,1 +access_team_treatment_pro,Treatment Professional Access for Teams,model_sports_team,group_sports_clinic_treatment_professional,1,1,1,0 access_team_portal,Portal Access for Teams,model_sports_team,base.group_portal,1,0,0,0 access_team_staff_user,User Access for Team Staff,model_sports_team_staff,group_sports_clinic_user,1,1,1,1 access_team_staff_portal,Portal Access for Team Staff,model_sports_team_staff,base.group_portal,1,0,0,0 @@ -27,6 +28,9 @@ access_injury_document_portal_read,Portal Read Access for Injury Documents,model access_injury_document_admin,Admin Access for Injury Documents,model_sports_injury_document,group_sports_clinic_admin,1,1,1,1 access_mail_activity_type_portal_tp,Portal TP Access for Activity Types,mail.model_mail_activity_type,bemade_sports_clinic.group_portal_treatment_professional,1,0,0,0 access_mail_activity_portal_tp,Portal TP Access for Activities,mail.model_mail_activity,bemade_sports_clinic.group_portal_treatment_professional,1,1,1,1 +access_mail_compose_message_portal,Portal Access for Mail Compose,mail.model_mail_compose_message,base.group_portal,1,1,1,0 +access_mail_compose_message_portal_tp,Portal TP Access for Mail Compose,mail.model_mail_compose_message,bemade_sports_clinic.group_portal_treatment_professional,1,1,1,0 +access_mail_compose_message_portal_coach,Portal Coach Access for Mail Compose,mail.model_mail_compose_message,bemade_sports_clinic.group_portal_team_coach,1,1,1,0 access_mail_alias_domain_portal_tp,Portal TP Access for Alias Domains,mail.model_mail_alias_domain,bemade_sports_clinic.group_portal_treatment_professional,1,0,0,0 access_mail_alias_portal_tp,Portal TP Access for Aliases,mail.model_mail_alias,bemade_sports_clinic.group_portal_treatment_professional,1,0,0,0 access_ir_model_portal_tp,Portal TP Access for Models,base.model_ir_model,bemade_sports_clinic.group_portal_treatment_professional,1,0,0,0 diff --git a/bemade_sports_clinic/security/sports_clinic_rules.xml b/bemade_sports_clinic/security/sports_clinic_rules.xml index 9cfe05b..16f83da 100644 --- a/bemade_sports_clinic/security/sports_clinic_rules.xml +++ b/bemade_sports_clinic/security/sports_clinic_rules.xml @@ -6,7 +6,7 @@ Restrict Team Staff Access to Their Players Only + eval="[(6, 0, [ref('base.group_portal'), ref('base.group_user'), ref('group_sports_clinic_treatment_professional')])]"/> @@ -19,7 +19,7 @@ Restrict Team Staff Access to Their Teams Only + eval="[(6, 0, [ref('base.group_portal'), ref('base.group_user'), ref('group_sports_clinic_treatment_professional')])]"/> @@ -32,7 +32,7 @@ Restrict Team Staff Access to Their Teams Only + eval="[(6, 0, [ref('base.group_portal'), ref('base.group_user'), ref('group_sports_clinic_treatment_professional')])]"/> @@ -47,7 +47,7 @@ Users - + diff --git a/bemade_sports_clinic/views/player_management_portal_templates.xml b/bemade_sports_clinic/views/player_management_portal_templates.xml index 7028d72..a1a4bed 100644 --- a/bemade_sports_clinic/views/player_management_portal_templates.xml +++ b/bemade_sports_clinic/views/player_management_portal_templates.xml @@ -82,10 +82,10 @@
- +