From b9de91080d6cb062eec56948dcb8d0dd62ae0362 Mon Sep 17 00:00:00 2001 From: Atharv Bhandare Date: Tue, 6 May 2025 11:01:00 +1000 Subject: [PATCH 1/8] Imported role_helpers to implement Role Based Access Controls --- app/api/entities/unit_entity.rb | 39 ++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/app/api/entities/unit_entity.rb b/app/api/entities/unit_entity.rb index efd50eb510..53767b8546 100644 --- a/app/api/entities/unit_entity.rb +++ b/app/api/entities/unit_entity.rb @@ -1,16 +1,22 @@ +require_relative '../../helpers/role_helpers' #Imported a helper module (role_helpers.rb) to centralize role-based access control (RBAC) logic. + module Entities class UnitEntity < Grape::Entity format_with(:date_only) do |date| date.strftime('%Y-%m-%d') end - def is_staff?(my_role) - [Role.tutor_id, Role.convenor_id, Role.admin_id, Role.auditor_id].include?(my_role.id) unless my_role.nil? - end + include RoleHelpers # Including the role helper methods for permission checks - def can_read_unit_config?(my_role) - [Role.convenor_id, Role.admin_id, Role.auditor_id].include?(my_role.id) unless my_role.nil? - end + # Previously these helper methods were uncommented directly. Leaving them commented to show intention for reusability. Replaced them with the centralized versions from the RoleHelpers module for consistency and maintainability. + + # def is_staff?(my_role) + # [Role.tutor_id, Role.convenor_id, Role.admin_id, Role.auditor_id].include?(my_role.id) unless my_role.nil? + # end + + # def can_read_unit_config?(my_role) + # [Role.convenor_id, Role.admin_id, Role.auditor_id].include?(my_role.id) unless my_role.nil? + # end expose :code expose :id @@ -28,31 +34,34 @@ def can_read_unit_config?(my_role) expose :description expose :teaching_period_id, expose_nil: false + # Replaced all inline is_staff? and can_read_unit_config? checks with RoleHelpers.is_staff? and RoleHelpers.can_read_unit_config? to delegate access control checks to the RoleHelpers module. + # Added conditional checks on all fields using RoleHelpers.is_staff? to ensure mutiple internal config fields are not accessible to non-staff users. + with_options(format_with: :date_only) do expose :start_date expose :end_date - expose :portfolio_auto_generation_date, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) }, expose_nil: false + expose :portfolio_auto_generation_date, unless: :summary_only, if: lambda { |unit, options| RoleHelpers.is_staff?(options[:my_role]) }, expose_nil: false end expose :active - expose :overseer_image_id, unless: :summary_only, if: lambda { |unit, options| can_read_unit_config?(options[:my_role]) } + expose :overseer_image_id, unless: :summary_only, if: lambda { |unit, options| RoleHelpers.can_read_unit_config?(options[:my_role]) } expose :assessment_enabled, unless: :summary_only - expose :auto_apply_extension_before_deadline, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) } - expose :send_notifications, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) } - expose :enable_sync_enrolments, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) } - expose :enable_sync_timetable, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) } - expose :draft_task_definition_id, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) } + expose :auto_apply_extension_before_deadline, unless: :summary_only, if: lambda { |unit, options| RoleHelpers.is_staff?(options[:my_role]) } + expose :send_notifications, unless: :summary_only, if: lambda { |unit, options| RoleHelpers.is_staff?(options[:my_role]) } + expose :enable_sync_enrolments, unless: :summary_only, if: lambda { |unit, options| RoleHelpers.is_staff?(options[:my_role]) } + expose :enable_sync_timetable, unless: :summary_only, if: lambda { |unit, options| RoleHelpers.is_staff?(options[:my_role]) } + expose :draft_task_definition_id, unless: :summary_only, if: lambda { |unit, options| RoleHelpers.is_staff?(options[:my_role]) } expose :allow_student_extension_requests, unless: :summary_only - expose :extension_weeks_on_resubmit_request, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) } + expose :extension_weeks_on_resubmit_request, unless: :summary_only, if: lambda { |unit, options| RoleHelpers.is_staff?(options[:my_role]) } expose :allow_student_change_tutorial, unless: :summary_only expose :learning_outcomes, using: LearningOutcomeEntity, as: :ilos, unless: :summary_only expose :tutorial_streams, using: TutorialStreamEntity, unless: :summary_only # Expose staff before tutorials, so that their details are available - expose :staff, using: UnitRoleEntity, unless: :summary_only + expose :staff, using: UnitRoleEntity, unless: :summary_only, if: lambda { |unit, options| RoleHelpers.is_staff?(options[:my_role]) } #Serializer Restriction: to hide the staff field for students expose :tutorials, using: TutorialEntity, unless: :summary_only # expose :tutorial_enrolments, using: TutorialEnrolmentEntity, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) } From 56a3ecce5ae7ce7e52ce726faa7c826bdae0c627 Mon Sep 17 00:00:00 2001 From: Atharv Bhandare Date: Tue, 6 May 2025 11:11:21 +1000 Subject: [PATCH 2/8] Reusing centralized role-checking logic by importing role_helpers module to implement RBAC on email, firstname, lastmame, username --- app/api/entities/user_entity.rb | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/app/api/entities/user_entity.rb b/app/api/entities/user_entity.rb index 1a1155e103..ebb7b6379c 100644 --- a/app/api/entities/user_entity.rb +++ b/app/api/entities/user_entity.rb @@ -1,11 +1,17 @@ +require_relative '../../helpers/role_helpers' #Reuses centralized role-checking logic module Entities class UserEntity < Grape::Entity + include RoleHelpers # Makes role-checking functions available + expose :id expose :student_id, unless: :minimal - expose :email - expose :first_name - expose :last_name - expose :username + + # Wrapped email, first_name, last_name, username to prevent exposure of sensitive staff data to unauthorized users (e.g., students) + expose :email, if: ->(_user, options) { RoleHelpers.is_staff?(options[:my_role]) } + expose :first_name, if: ->(_user, options) { RoleHelpers.is_staff?(options[:my_role]) } + expose :last_name, if: ->(_user, options) { RoleHelpers.is_staff?(options[:my_role]) } + expose :username, if: ->(_user, options) { RoleHelpers.is_staff?(options[:my_role]) } + expose :nickname expose :receive_task_notifications, unless: :minimal expose :receive_portfolio_notifications, unless: :minimal From a35aea12f4c23408e7c84a298fc66dd8f0d02dee Mon Sep 17 00:00:00 2001 From: Atharv Bhandare Date: Tue, 6 May 2025 11:30:36 +1000 Subject: [PATCH 3/8] Add role-based checks in UnitRoleEntity to prevent students from accessing full staff details --- app/api/entities/unit_role_entity.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/app/api/entities/unit_role_entity.rb b/app/api/entities/unit_role_entity.rb index 08159ea44e..0aba12d882 100644 --- a/app/api/entities/unit_role_entity.rb +++ b/app/api/entities/unit_role_entity.rb @@ -2,7 +2,15 @@ module Entities class UnitRoleEntity < Grape::Entity expose :id expose :role do |unit_role, options| unit_role.role.name end - expose :user, using: Entities::Minimal::MinimalUserEntity + + # Replaced static MinimalUserEntity with conditional branching to prevent students from seeing full staff details (violates least privilege) + expose :user, using: Entities::UserEntity, if: lambda { |_unit_role, options| + Entities::UnitEntity.is_staff?(options[:my_role]) + } + # Used Entities::UnitEntity.is_staff? for permission check to use centralized and reusable RBAC logic + expose :user, using: Entities::Minimal::MinimalUserEntity, unless: lambda { |_unit_role, options| + Entities::UnitEntity.is_staff?(options[:my_role]) + } expose :unit, using: Entities::Minimal::MinimalUnitEntity, unless: :in_unit end end From b0b1cef44c172b38632423b7e018782c6d457f4e Mon Sep 17 00:00:00 2001 From: Atharv Bhandare Date: Tue, 6 May 2025 12:33:01 +1000 Subject: [PATCH 4/8] Add my_role context to UnitRole serializer to enforce RBAC during API response serialization. --- app/api/unit_roles_api.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/api/unit_roles_api.rb b/app/api/unit_roles_api.rb index b59a69f96c..18636c8db3 100644 --- a/app/api/unit_roles_api.rb +++ b/app/api/unit_roles_api.rb @@ -90,6 +90,7 @@ class UnitRolesApi < Grape::API end unit_role.update!(unit_role_parameters) - present unit_role, with: Entities::UnitRoleEntity, in_unit: true + present unit_role, with: Entities::UnitRoleEntity, in_unit: true, my_role: current_user.role_for(unit_role.unit) + #Without passing my_role, the serializer (Grape Entity class) has no way of knowing the current user’s privileges so it would either over-expose data or fail to restrict access properly, in this case enforcing role based data exposure instead of conditionally exposing. end end From 358a9f15936f31c99c4a45ee3eda3cfddb1fc398 Mon Sep 17 00:00:00 2001 From: Atharv Bhandare Date: Tue, 6 May 2025 12:42:53 +1000 Subject: [PATCH 5/8] Add user: current_user to unit serializer to enable role-based filtering in UnitEntity for RBAC enforcement. --- app/api/units_api.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/api/units_api.rb b/app/api/units_api.rb index 1dd92a3d16..279960963d 100644 --- a/app/api/units_api.rb +++ b/app/api/units_api.rb @@ -60,7 +60,8 @@ class UnitsApi < Grape::API # Unit uses user from thread to limit exposure # my_role = unit.role_for(current_user) - present unit, with: Entities::UnitEntity, my_role: my_role, in_unit: true + present unit, with: Entities::UnitEntity, my_role: my_role, user: current_user, in_unit: true + # added the user: current_user key-value pair by explicitly passing current_user to UnitEntity serializer which enables RBAC checks in serialization, ensuring sensitive fields like staff details are hidden from unauthorized users end desc 'Update unit' From c1593c2d915fbcc3e7926061c2eb62cd83023504 Mon Sep 17 00:00:00 2001 From: atharv02-git Date: Tue, 6 May 2025 13:02:34 +1000 Subject: [PATCH 6/8] Added role_helpers module to centralize and reuse RBAC logic across different api entities --- app/helpers/role_helpers.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 app/helpers/role_helpers.rb diff --git a/app/helpers/role_helpers.rb b/app/helpers/role_helpers.rb new file mode 100644 index 0000000000..553b2a6298 --- /dev/null +++ b/app/helpers/role_helpers.rb @@ -0,0 +1,10 @@ +# created the role_helpers.rb module to centralize and reuse role-based access control (RBAC) logic across multiple entities and API endpoints +module RoleHelpers + def self.is_staff?(my_role) + [Role.tutor_id, Role.convenor_id, Role.admin_id, Role.auditor_id].include?(my_role.id) unless my_role.nil? + end + + def self.can_read_unit_config?(my_role) + [Role.convenor_id, Role.admin_id, Role.auditor_id].include?(my_role.id) unless my_role.nil? + end +end From a294e9d9207f8e8094102fa56f314bb051c2c098 Mon Sep 17 00:00:00 2001 From: Atharv Bhandare Date: Thu, 8 May 2025 15:41:34 +1000 Subject: [PATCH 7/8] Restoring all the changes as per 8.0.x --- app/api/entities/unit_entity.rb | 39 +++++++++++----------------- app/api/entities/unit_role_entity.rb | 10 +------ app/api/entities/user_entity.rb | 14 +++------- app/api/unit_roles_api.rb | 3 +-- app/api/units_api.rb | 3 +-- app/helpers/role_helpers.rb | 10 ------- 6 files changed, 22 insertions(+), 57 deletions(-) delete mode 100644 app/helpers/role_helpers.rb diff --git a/app/api/entities/unit_entity.rb b/app/api/entities/unit_entity.rb index 53767b8546..efd50eb510 100644 --- a/app/api/entities/unit_entity.rb +++ b/app/api/entities/unit_entity.rb @@ -1,22 +1,16 @@ -require_relative '../../helpers/role_helpers' #Imported a helper module (role_helpers.rb) to centralize role-based access control (RBAC) logic. - module Entities class UnitEntity < Grape::Entity format_with(:date_only) do |date| date.strftime('%Y-%m-%d') end - include RoleHelpers # Including the role helper methods for permission checks - - # Previously these helper methods were uncommented directly. Leaving them commented to show intention for reusability. Replaced them with the centralized versions from the RoleHelpers module for consistency and maintainability. - - # def is_staff?(my_role) - # [Role.tutor_id, Role.convenor_id, Role.admin_id, Role.auditor_id].include?(my_role.id) unless my_role.nil? - # end + def is_staff?(my_role) + [Role.tutor_id, Role.convenor_id, Role.admin_id, Role.auditor_id].include?(my_role.id) unless my_role.nil? + end - # def can_read_unit_config?(my_role) - # [Role.convenor_id, Role.admin_id, Role.auditor_id].include?(my_role.id) unless my_role.nil? - # end + def can_read_unit_config?(my_role) + [Role.convenor_id, Role.admin_id, Role.auditor_id].include?(my_role.id) unless my_role.nil? + end expose :code expose :id @@ -34,34 +28,31 @@ class UnitEntity < Grape::Entity expose :description expose :teaching_period_id, expose_nil: false - # Replaced all inline is_staff? and can_read_unit_config? checks with RoleHelpers.is_staff? and RoleHelpers.can_read_unit_config? to delegate access control checks to the RoleHelpers module. - # Added conditional checks on all fields using RoleHelpers.is_staff? to ensure mutiple internal config fields are not accessible to non-staff users. - with_options(format_with: :date_only) do expose :start_date expose :end_date - expose :portfolio_auto_generation_date, unless: :summary_only, if: lambda { |unit, options| RoleHelpers.is_staff?(options[:my_role]) }, expose_nil: false + expose :portfolio_auto_generation_date, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) }, expose_nil: false end expose :active - expose :overseer_image_id, unless: :summary_only, if: lambda { |unit, options| RoleHelpers.can_read_unit_config?(options[:my_role]) } + expose :overseer_image_id, unless: :summary_only, if: lambda { |unit, options| can_read_unit_config?(options[:my_role]) } expose :assessment_enabled, unless: :summary_only - expose :auto_apply_extension_before_deadline, unless: :summary_only, if: lambda { |unit, options| RoleHelpers.is_staff?(options[:my_role]) } - expose :send_notifications, unless: :summary_only, if: lambda { |unit, options| RoleHelpers.is_staff?(options[:my_role]) } - expose :enable_sync_enrolments, unless: :summary_only, if: lambda { |unit, options| RoleHelpers.is_staff?(options[:my_role]) } - expose :enable_sync_timetable, unless: :summary_only, if: lambda { |unit, options| RoleHelpers.is_staff?(options[:my_role]) } - expose :draft_task_definition_id, unless: :summary_only, if: lambda { |unit, options| RoleHelpers.is_staff?(options[:my_role]) } + expose :auto_apply_extension_before_deadline, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) } + expose :send_notifications, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) } + expose :enable_sync_enrolments, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) } + expose :enable_sync_timetable, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) } + expose :draft_task_definition_id, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) } expose :allow_student_extension_requests, unless: :summary_only - expose :extension_weeks_on_resubmit_request, unless: :summary_only, if: lambda { |unit, options| RoleHelpers.is_staff?(options[:my_role]) } + expose :extension_weeks_on_resubmit_request, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) } expose :allow_student_change_tutorial, unless: :summary_only expose :learning_outcomes, using: LearningOutcomeEntity, as: :ilos, unless: :summary_only expose :tutorial_streams, using: TutorialStreamEntity, unless: :summary_only # Expose staff before tutorials, so that their details are available - expose :staff, using: UnitRoleEntity, unless: :summary_only, if: lambda { |unit, options| RoleHelpers.is_staff?(options[:my_role]) } #Serializer Restriction: to hide the staff field for students + expose :staff, using: UnitRoleEntity, unless: :summary_only expose :tutorials, using: TutorialEntity, unless: :summary_only # expose :tutorial_enrolments, using: TutorialEnrolmentEntity, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) } diff --git a/app/api/entities/unit_role_entity.rb b/app/api/entities/unit_role_entity.rb index 0aba12d882..08159ea44e 100644 --- a/app/api/entities/unit_role_entity.rb +++ b/app/api/entities/unit_role_entity.rb @@ -2,15 +2,7 @@ module Entities class UnitRoleEntity < Grape::Entity expose :id expose :role do |unit_role, options| unit_role.role.name end - - # Replaced static MinimalUserEntity with conditional branching to prevent students from seeing full staff details (violates least privilege) - expose :user, using: Entities::UserEntity, if: lambda { |_unit_role, options| - Entities::UnitEntity.is_staff?(options[:my_role]) - } - # Used Entities::UnitEntity.is_staff? for permission check to use centralized and reusable RBAC logic - expose :user, using: Entities::Minimal::MinimalUserEntity, unless: lambda { |_unit_role, options| - Entities::UnitEntity.is_staff?(options[:my_role]) - } + expose :user, using: Entities::Minimal::MinimalUserEntity expose :unit, using: Entities::Minimal::MinimalUnitEntity, unless: :in_unit end end diff --git a/app/api/entities/user_entity.rb b/app/api/entities/user_entity.rb index ebb7b6379c..1a1155e103 100644 --- a/app/api/entities/user_entity.rb +++ b/app/api/entities/user_entity.rb @@ -1,17 +1,11 @@ -require_relative '../../helpers/role_helpers' #Reuses centralized role-checking logic module Entities class UserEntity < Grape::Entity - include RoleHelpers # Makes role-checking functions available - expose :id expose :student_id, unless: :minimal - - # Wrapped email, first_name, last_name, username to prevent exposure of sensitive staff data to unauthorized users (e.g., students) - expose :email, if: ->(_user, options) { RoleHelpers.is_staff?(options[:my_role]) } - expose :first_name, if: ->(_user, options) { RoleHelpers.is_staff?(options[:my_role]) } - expose :last_name, if: ->(_user, options) { RoleHelpers.is_staff?(options[:my_role]) } - expose :username, if: ->(_user, options) { RoleHelpers.is_staff?(options[:my_role]) } - + expose :email + expose :first_name + expose :last_name + expose :username expose :nickname expose :receive_task_notifications, unless: :minimal expose :receive_portfolio_notifications, unless: :minimal diff --git a/app/api/unit_roles_api.rb b/app/api/unit_roles_api.rb index 18636c8db3..b59a69f96c 100644 --- a/app/api/unit_roles_api.rb +++ b/app/api/unit_roles_api.rb @@ -90,7 +90,6 @@ class UnitRolesApi < Grape::API end unit_role.update!(unit_role_parameters) - present unit_role, with: Entities::UnitRoleEntity, in_unit: true, my_role: current_user.role_for(unit_role.unit) - #Without passing my_role, the serializer (Grape Entity class) has no way of knowing the current user’s privileges so it would either over-expose data or fail to restrict access properly, in this case enforcing role based data exposure instead of conditionally exposing. + present unit_role, with: Entities::UnitRoleEntity, in_unit: true end end diff --git a/app/api/units_api.rb b/app/api/units_api.rb index 279960963d..1dd92a3d16 100644 --- a/app/api/units_api.rb +++ b/app/api/units_api.rb @@ -60,8 +60,7 @@ class UnitsApi < Grape::API # Unit uses user from thread to limit exposure # my_role = unit.role_for(current_user) - present unit, with: Entities::UnitEntity, my_role: my_role, user: current_user, in_unit: true - # added the user: current_user key-value pair by explicitly passing current_user to UnitEntity serializer which enables RBAC checks in serialization, ensuring sensitive fields like staff details are hidden from unauthorized users + present unit, with: Entities::UnitEntity, my_role: my_role, in_unit: true end desc 'Update unit' diff --git a/app/helpers/role_helpers.rb b/app/helpers/role_helpers.rb deleted file mode 100644 index 553b2a6298..0000000000 --- a/app/helpers/role_helpers.rb +++ /dev/null @@ -1,10 +0,0 @@ -# created the role_helpers.rb module to centralize and reuse role-based access control (RBAC) logic across multiple entities and API endpoints -module RoleHelpers - def self.is_staff?(my_role) - [Role.tutor_id, Role.convenor_id, Role.admin_id, Role.auditor_id].include?(my_role.id) unless my_role.nil? - end - - def self.can_read_unit_config?(my_role) - [Role.convenor_id, Role.admin_id, Role.auditor_id].include?(my_role.id) unless my_role.nil? - end -end From df0bdcadaef030575ac72dce692d02de7810ca22 Mon Sep 17 00:00:00 2001 From: Atharv Bhandare Date: Thu, 8 May 2025 16:16:17 +1000 Subject: [PATCH 8/8] filtering out the staff_data object for non_staff_roles --- app/api/entities/unit_entity.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/api/entities/unit_entity.rb b/app/api/entities/unit_entity.rb index efd50eb510..d9a45c07e3 100644 --- a/app/api/entities/unit_entity.rb +++ b/app/api/entities/unit_entity.rb @@ -52,7 +52,7 @@ def can_read_unit_config?(my_role) expose :tutorial_streams, using: TutorialStreamEntity, unless: :summary_only # Expose staff before tutorials, so that their details are available - expose :staff, using: UnitRoleEntity, unless: :summary_only + expose :staff, using: UnitRoleEntity, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) } # filtering out the staff_data object for non_staff_roles expose :tutorials, using: TutorialEntity, unless: :summary_only # expose :tutorial_enrolments, using: TutorialEnrolmentEntity, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) }