diff --git a/app/controllers/system/admin/marketplace_access_blocks_controller.rb b/app/controllers/system/admin/marketplace_access_blocks_controller.rb new file mode 100644 index 00000000000..3bb19d952a6 --- /dev/null +++ b/app/controllers/system/admin/marketplace_access_blocks_controller.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true +class System::Admin::MarketplaceAccessBlocksController < System::Admin::Controller + def create + block = Course::Assessment::Marketplace::AccessBlock.new( + user_id: params[:user_id], creator: current_user + ) + if block.save + render json: { id: block.id, userId: block.user_id }, status: :ok + else + render json: { errors: block.errors.full_messages.to_sentence }, status: :bad_request + end + end + + def destroy + block = Course::Assessment::Marketplace::AccessBlock.find(params[:id]) + if block.destroy + head :ok + else + render json: { errors: block.errors.full_messages.to_sentence }, status: :bad_request + end + end +end diff --git a/app/controllers/system/admin/marketplace_access_controller.rb b/app/controllers/system/admin/marketplace_access_controller.rb new file mode 100644 index 00000000000..50f21b0da89 --- /dev/null +++ b/app/controllers/system/admin/marketplace_access_controller.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true +class System::Admin::MarketplaceAccessController < System::Admin::Controller + def index + query = Course::Assessment::Marketplace::AccessListQuery.new + @rows = query.rows + @summary = query.summary + end +end diff --git a/app/controllers/system/admin/marketplace_allowlist_rules_controller.rb b/app/controllers/system/admin/marketplace_allowlist_rules_controller.rb new file mode 100644 index 00000000000..80d4b8ff007 --- /dev/null +++ b/app/controllers/system/admin/marketplace_allowlist_rules_controller.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true +class System::Admin::MarketplaceAllowlistRulesController < System::Admin::Controller + # `preview` is a collection action with no id, so CanCan's default loader would try + # `find(params[:id])`. It builds its own unsaved rule; System::Admin::Controller's + # `authorize_admin` already gates the whole controller. + load_and_authorize_resource :allowlist_rule, + class: 'Course::Assessment::Marketplace::AllowlistRule', + parent: false, except: [:preview] + + def index + # "Everyone" is a page-level mode, not a table row: expose its presence as `@everyone_rule` + # and show only the scoped rules in the table. + @everyone_rule = @allowlist_rules.rule_type_everyone.first + @allowlist_rules = @allowlist_rules.where.not(rule_type: :everyone).includes(:user, :instance) + end + + def create + if @allowlist_rule.save + # `render partial:` (not `render 'rule'`) — the view is the `_rule` partial. Mirrors + # System::Admin::AnnouncementsController#create (`render partial: '.../announcement_data'`). + render partial: 'rule', locals: { rule: @allowlist_rule }, status: :ok + else + render json: { errors: @allowlist_rule.errors.full_messages.to_sentence }, status: :bad_request + end + end + + def preview + rule = Course::Assessment::Marketplace::AllowlistRule.new(allowlist_rule_params) + unless rule.valid? + render json: { errors: rule.errors.full_messages.to_sentence }, status: :bad_request + return + end + + query = Course::Assessment::Marketplace::RulePreviewQuery.new(rule) + @rows = query.rows + @summary = query.summary + end + + def destroy + if @allowlist_rule.destroy + head :ok + else + render json: { errors: @allowlist_rule.errors.full_messages.to_sentence }, status: :bad_request + end + end + + private + + def allowlist_rule_params + params.require(:allowlist_rule).permit(:rule_type, :user_id, :instance_id, :email_domain, :email) + end +end diff --git a/app/helpers/system/admin/marketplace_access_helper.rb b/app/helpers/system/admin/marketplace_access_helper.rb new file mode 100644 index 00000000000..364d0cf864c --- /dev/null +++ b/app/helpers/system/admin/marketplace_access_helper.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true +module System::Admin::MarketplaceAccessHelper + # The value half of a rule's label ("Email domain · "), for the audit list's reason column. + # @param [Course::Assessment::Marketplace::AllowlistRule] rule + # @return [String, nil] + def marketplace_rule_label_value(rule) + case rule.rule_type + when 'user' then rule.user&.name + when 'instance' then rule.instance&.name + when 'email_domain' then rule.email_domain + end + end +end diff --git a/app/models/components/course/assessment_marketplace_ability_component.rb b/app/models/components/course/assessment_marketplace_ability_component.rb index e7a7972085c..45dc1e7922b 100644 --- a/app/models/components/course/assessment_marketplace_ability_component.rb +++ b/app/models/components/course/assessment_marketplace_ability_component.rb @@ -4,12 +4,46 @@ module Course::AssessmentMarketplaceAbilityComponent def define_permissions allow_admins_publish_to_marketplace if user&.administrator? - allow_managers_access_marketplace if course_user&.manager_or_owner? + # System admins keep marketplace access via `can :manage, :all` (Ability#initialize); do not + # emit a `cannot` for them or it would revoke that. For everyone else, access is per-person. + if course && !user&.administrator? + if can_access_marketplace? + allow_managers_access_marketplace + else + # `Course::CourseAbilityComponent` grants managers/owners a blanket `can :manage, Course`, + # which (CanCan's `:manage` matches any action) would otherwise satisfy `:access_marketplace` + # regardless of the allow-list. This component runs after that one in the `define_permissions` + # super chain, so a `cannot` here takes precedence. This line is load-bearing. + cannot :access_marketplace, Course, id: course.id + end + end super end private + # Access is per-person, not per-current-course-role: anyone who is baseline-capable (manages/owns + # >=1 course anywhere, OR is an instructor/administrator in any instance) and passes the allow-list + # may browse, whatever their role in the course they are viewing. + def can_access_marketplace? + marketplace_baseline_capable? && marketplace_visible_to_user? + end + + # The two peer baseline capabilities for the marketplace. Either qualifies; the allow-list narrows. + def marketplace_baseline_capable? + user&.course_manager_or_owner? || user&.instance_instructor_or_administrator? + end + + # Part of the TEMPORARY allow-list gate (see the retirement seam on `can_access_marketplace?`). + # When the allow-list is retired this whole method is deleted; the block check goes with it. + def marketplace_visible_to_user? + return true if user&.administrator? + + Course::Assessment::Marketplace::AllowlistRule.grants_access?(user) && + !Course::Assessment::Marketplace::AccessBlock.blocked?(user) + end + + def allow_admins_publish_to_marketplace can :publish_to_marketplace, Course::Assessment end @@ -23,4 +57,4 @@ def allow_managers_access_marketplace assessment.marketplace_listing&.published? || false end end -end +end \ No newline at end of file diff --git a/app/models/course/assessment/marketplace/access_block.rb b/app/models/course/assessment/marketplace/access_block.rb new file mode 100644 index 00000000000..03841d618a6 --- /dev/null +++ b/app/models/course/assessment/marketplace/access_block.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true +class Course::Assessment::Marketplace::AccessBlock < ApplicationRecord + belongs_to :user, inverse_of: false + belongs_to :creator, class_name: 'User', inverse_of: false + + # Paired with the DB unique index on user_id: a user is blocked at most once. + validates :user_id, uniqueness: true + + # Whether +user+ has been individually disabled from the marketplace. Global (not tenant-scoped), + # mirroring AllowlistRule. + # @param [User] user + # @return [Boolean] + def self.blocked?(user) + return false unless user + + where(user_id: user.id).exists? + end + + # @return [Array] user ids of every block (for per-page status annotation). + def self.blocked_user_ids + pluck(:user_id) + end +end diff --git a/app/models/course/assessment/marketplace/access_list_query.rb b/app/models/course/assessment/marketplace/access_list_query.rb new file mode 100644 index 00000000000..d0480acc479 --- /dev/null +++ b/app/models/course/assessment/marketplace/access_list_query.rb @@ -0,0 +1,133 @@ +# frozen_string_literal: true +# Computes the marketplace access audit list: every user who is baseline-capable (manages/owns >=1 +# course, OR is an instructor/administrator in any instance) AND cleared by the allow-list, PLUS +# every individually blocked user regardless of rule match — an orphaned block must stay visible and +# clearable. Blocked users are INCLUDED and flagged. Not paginated server-side - the eligible set is +# bounded (managers + instance staff) and the frontend paginates/searches client-side, matching the +# rules page which also fetches its whole list at once. +class Course::Assessment::Marketplace::AccessListQuery + AllowlistRule = Course::Assessment::Marketplace::AllowlistRule + AccessBlock = Course::Assessment::Marketplace::AccessBlock + RuleMatchQuery = Course::Assessment::Marketplace::RuleMatchQuery + + # `allowed_by_rules` holds EVERY rule matching the user, not one precedence winner: the admin uses + # it to answer "if I delete this rule, who loses access?", and one reason answers that wrongly. + Row = Struct.new(:user, :course_count, :instance_role, :allowed_by_rules, :block_id, + :system_admin, keyword_init: true) do + def blocked? + block_id.present? + end + + def system_admin? + system_admin.present? + end + end + + # @return [Array] + def rows + @rows ||= annotate(listed_users.to_a) + end + + # @return [Hash] + def summary + { + total_with_access: rows.count { |row| !row.blocked? }, + total_blocked: rows.count(&:blocked?), + open_to_everyone: everyone? + } + end + + # Baseline-eligible users the allow-list currently clears. A block does not remove someone from + # this set — being blocked is a separate decision layered on top of being allowed. + # @return [Set] + def allowed_user_ids + # System admins hold a blanket `can :manage, :all`, so they have the marketplace whatever the + # rules say. This table is the audit source of truth, so they are always in it — omitting them + # would show an admin as having no access while they in fact bypass every gate. + @allowed_user_ids ||= (everyone? ? baseline_ids.to_set : rules_by_user.keys.to_set) | admin_ids + end + + private + + # Batches every per-user annotation into one query each, keyed by user id. + def annotate(users) + ids = users.map(&:id) + counts = managed_course_counts(ids) + staff = instance_staff_roles(ids) + block_ids = block_ids_by_user(ids) + + users.map do |user| + Row.new(user: user, course_count: counts[user.id] || 0, + instance_role: staff[user.id], allowed_by_rules: rules_by_user[user.id] || [], + block_id: block_ids[user.id], system_admin: admin_ids.include?(user.id)) + end + end + + def managed_course_counts(ids) + CourseUser.managers.where(user_id: ids).group(:user_id).count + end + + def block_ids_by_user(ids) + AccessBlock.where(user_id: ids).pluck(:user_id, :id).to_h + end + + def blocked_ids + @blocked_ids ||= AccessBlock.pluck(:user_id).to_set + end + + def listed_users + User.where(id: (allowed_user_ids | blocked_ids).to_a).includes(:emails).order(:name) + end + + def admin_ids + @admin_ids ||= User.administrator.pluck(:id).to_set + end + + def baseline_ids + @baseline_ids ||= baseline_scope.pluck(:id) + end + + # CourseUser is not tenant-scoped ("any course"); InstanceUser IS, so .unscoped for "any instance". + def baseline_scope + User.where(id: CourseUser.managers.select(:user_id)). + or(User.where(id: instance_staff_scope.select(:user_id))). + or(User.administrator) + end + + def instance_staff_scope + InstanceUser.unscoped.where(role: [:instructor, :administrator]) + end + + def everyone? + return @everyone if defined?(@everyone) + + @everyone = AllowlistRule.rule_type_everyone.exists? + end + + # An `everyone` rule is a page-level mode, not a per-row reason, so it contributes no scoped rules. + def scoped_rules + @scoped_rules ||= if everyone? + [] + else + # `user`/`instance` are read when labelling each row's reasons; preload them + # once here rather than once per rule per row. + AllowlistRule.where.not(rule_type: :everyone). + includes(:user, :instance).order(:id).to_a + end + end + + # user id => [AllowlistRule], every rule matching that user, in rules-table order. + def rules_by_user + @rules_by_user ||= scoped_rules.each_with_object({}) do |rule, map| + RuleMatchQuery.new(rule).user_ids_within(baseline_ids).each do |id| + (map[id] ||= []) << rule + end + end + end + + def instance_staff_roles(ids) + InstanceUser.unscoped.where(user_id: ids, role: [:instructor, :administrator]). + group(:user_id).maximum(:role). + transform_values { |role| InstanceUser.roles.key(role) } + end +end diff --git a/app/models/course/assessment/marketplace/allowlist_rule.rb b/app/models/course/assessment/marketplace/allowlist_rule.rb new file mode 100644 index 00000000000..a5c3317f169 --- /dev/null +++ b/app/models/course/assessment/marketplace/allowlist_rule.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true +class Course::Assessment::Marketplace::AllowlistRule < ApplicationRecord + enum :rule_type, + { user: 0, instance: 1, email_domain: 2, everyone: 3 }, + prefix: true + + belongs_to :user, class_name: 'User', inverse_of: false, optional: true + belongs_to :instance, inverse_of: false, optional: true + + # Transient: the admin form identifies a `user` rule by email (user IDs are not shown anywhere + # in the admin panel). Resolved to the owning user before validation; the stored row keeps the + # `user_id` FK, so the rule means "this person" even if they later change email. + attr_accessor :email + + before_validation :resolve_user_from_email, if: -> { rule_type_user? && email.present? } + + # When an email was supplied, `resolve_user_from_email` reports its own failure; skip the generic + # presence check in that path so the message is exactly "No user with that email." (not a pair). + before_validation :normalize_email_domain, if: :rule_type_email_domain? + + validates :user, presence: true, if: -> { rule_type_user? && email.blank? } + validates :instance, presence: true, if: :rule_type_instance? + validates :email_domain, presence: true, if: :rule_type_email_domain? + + # Identical rules grant nothing extra and make the rules table unreadable. Each is paired with a + # partial unique index. `scope: :rule_type` matters on the unresolved-email path: a user rule + # whose email matched nobody keeps user_id NULL, and Rails checks that as `user_id IS NULL`, + # which matches every instance and email-domain rule — reporting a bogus duplicate on top of the + # real "No user with that email." Scoping confines the check to rules of the same type. + validates :user_id, uniqueness: { scope: :rule_type, message: 'already has a rule.' }, + if: :rule_type_user? + validates :instance_id, uniqueness: { scope: :rule_type, message: 'already has a rule.' }, + if: :rule_type_instance? + validates :email_domain, uniqueness: { scope: :rule_type, message: 'already has a rule.' }, + if: :rule_type_email_domain? + # "Everyone" is the widest rule; only one may exist. Paired with a partial unique index. + validates :rule_type, uniqueness: true, if: :rule_type_everyone? + + # Whether the marketplace is visible to +user+ per the allow-list. The rules table itself is + # global (not tenant-scoped), but `user.instance_users` IS tenant-scoped (acts_as_tenant), so + # in a request an `instance` rule matches only while browsing the allow-listed instance — + # it grants that instance's users access *there*, not membership-based access everywhere. + # An `everyone` rule grants every authenticated user (the `nil` guard still excludes anonymous). + # Baseline (manager/owner OR instructor/admin) is checked separately in the ability component. + # @param [User] user + # @return [Boolean] + def self.grants_access?(user) + return false unless user + + rule_type_everyone.exists? || + rule_type_user.where(user_id: user.id).exists? || + rule_type_instance.where(instance_id: user.instance_users.select(:instance_id)).exists? || + email_domain_matches?(user) + end + + # @param [User] user + # @return [Boolean] + def self.email_domain_matches?(user) + domains = user.emails.pluck(:email).filter_map { |e| e.split('@').last&.downcase }.uniq + return false if domains.empty? + + rule_type_email_domain.where('LOWER(email_domain) IN (?)', domains).exists? + end + + private + + # Matching is case-insensitive everywhere, so the stored form is normalized on write. This keeps + # the uniqueness index a plain column comparison instead of a functional LOWER() index. + def normalize_email_domain + self.email_domain = email_domain&.strip&.downcase + end + + def resolve_user_from_email + self.user = User.with_email_addresses([email.strip.downcase]).first + errors.add(:base, 'No user with that email.') if user.nil? + end +end diff --git a/app/models/course/assessment/marketplace/rule_match_query.rb b/app/models/course/assessment/marketplace/rule_match_query.rb new file mode 100644 index 00000000000..9722c373364 --- /dev/null +++ b/app/models/course/assessment/marketplace/rule_match_query.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true +# Which baseline-eligible users does a single allow-list rule match? The rule may be unsaved, so the +# admin can preview a rule's effect before adding it. This is the one place that knows each rule +# type's matching semantics; AccessListQuery and the preview endpoint both go through it. +# +# Deliberately NOT the same as AllowlistRule.grants_access?, which reads the tenant-scoped +# `user.instance_users` at request time. Here `instance` rules match globally via +# InstanceUser.unscoped, because the audit list is not browsing any one instance. +class Course::Assessment::Marketplace::RuleMatchQuery + # @param [Course::Assessment::Marketplace::AllowlistRule] rule persisted or in-memory + def initialize(rule) + @rule = rule + end + + # @param [Array] candidate_user_ids the users to test + # @return [Set] the subset of +candidate_user_ids+ this rule matches + def user_ids_within(candidate_user_ids) + return Set.new if candidate_user_ids.empty? + + case @rule.rule_type + when 'everyone' then candidate_user_ids.to_set + when 'user' then matched_user(candidate_user_ids) + when 'instance' then matched_instance_members(candidate_user_ids) + when 'email_domain' then matched_domain_holders(candidate_user_ids) + else Set.new + end + end + + private + + def matched_user(ids) + (@rule.user_id.present? && ids.include?(@rule.user_id)) ? Set[@rule.user_id] : Set.new + end + + def matched_instance_members(ids) + return Set.new if @rule.instance_id.blank? + + InstanceUser.unscoped.where(user_id: ids, instance_id: @rule.instance_id). + pluck(:user_id).to_set + end + + def matched_domain_holders(ids) + domain = @rule.email_domain&.strip&.downcase + return Set.new if domain.blank? + + User::Email.where.not(confirmed_at: nil).where(user_id: ids). + where('LOWER(SPLIT_PART(email, ?, 2)) = ?', '@', domain). + pluck(:user_id).to_set + end +end diff --git a/app/models/course/assessment/marketplace/rule_preview_query.rb b/app/models/course/assessment/marketplace/rule_preview_query.rb new file mode 100644 index 00000000000..6e73cc3a40e --- /dev/null +++ b/app/models/course/assessment/marketplace/rule_preview_query.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true +# Answers "if I add this rule, who gets access?" for a rule that has not been saved, so the admin +# sees the effect before committing. Persists nothing. +class Course::Assessment::Marketplace::RulePreviewQuery + AccessBlock = Course::Assessment::Marketplace::AccessBlock + AccessListQuery = Course::Assessment::Marketplace::AccessListQuery + RuleMatchQuery = Course::Assessment::Marketplace::RuleMatchQuery + + Row = Struct.new(:user, :course_count, :instance_role, :already_has_access, :blocked, + keyword_init: true) + + # @param [Course::Assessment::Marketplace::AllowlistRule] rule an unsaved, valid rule + def initialize(rule) + @rule = rule + @access_list = AccessListQuery.new + end + + # @return [Array] + def rows + # Blocked first, then already-has-access, then the newly granted, each group still by name: a + # rule can match hundreds, and the people this rule does NOT newly reach are the only reason to + # read the list at all — buried on page 14 of an alphabetical list they may as well not be + # shown. A stable sort_by on the group rank alone, so the name order the database chose + # survives inside each group. + @rows ||= annotate(matched_users.to_a). + each_with_index.sort_by { |row, index| [group_rank(row), index] }.map(&:first) + end + + # @return [Hash] + def summary + { + matched_count: rows.size, + # A rule grants nothing to someone another rule already clears, and nothing at all to a + # blocked user — adding a rule does not unblock anyone. + new_count: rows.count { |row| !row.already_has_access && !row.blocked }, + blocked_count: rows.count(&:blocked), + open_to_everyone: @access_list.summary[:open_to_everyone] + } + end + + private + + # Same precedence as the row's status marker, which shows Blocked over already-has-access. + def group_rank(row) + return 0 if row.blocked + return 1 if row.already_has_access + + 2 + end + + def matched_ids + @matched_ids ||= RuleMatchQuery.new(@rule).user_ids_within(baseline_ids) + end + + # Only baseline-eligible staff can ever reach the marketplace, so a rule matching anyone else + # grants nothing and must not be counted. + def baseline_ids + @baseline_ids ||= User.where(id: CourseUser.managers.select(:user_id)). + or(User.where(id: instance_staff_scope.select(:user_id))).pluck(:id) + end + + def instance_staff_scope + InstanceUser.unscoped.where(role: [:instructor, :administrator]) + end + + def matched_users + User.where(id: matched_ids.to_a).includes(:emails).order(:name) + end + + def annotate(users) + ids = users.map(&:id) + counts = CourseUser.managers.where(user_id: ids).group(:user_id).count + staff = instance_staff_roles(ids) + allowed = @access_list.allowed_user_ids + blocked = AccessBlock.where(user_id: ids).pluck(:user_id).to_set + + users.map { |user| build_row(user, counts, staff, allowed, blocked) } + end + + def build_row(user, counts, staff, allowed, blocked) + Row.new(user: user, course_count: counts[user.id] || 0, instance_role: staff[user.id], + already_has_access: allowed.include?(user.id), blocked: blocked.include?(user.id)) + end + + def instance_staff_roles(ids) + InstanceUser.unscoped.where(user_id: ids, role: [:instructor, :administrator]). + group(:user_id).maximum(:role). + transform_values { |role| InstanceUser.roles.key(role) } + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 75aae3fc495..be7e37d6790 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -76,6 +76,22 @@ def deleted has_one :cikgo_user, dependent: :destroy, inverse_of: :user + # Both tables FK to users with no ON DELETE, so without these the admin panel's delete-user + # action dies with PG::ForeignKeyViolation for anyone who is allow-listed or blocked. Destroying + # is the right semantic for both: each row is *about* this user and means nothing without them. + has_many :marketplace_allowlist_rules, class_name: 'Course::Assessment::Marketplace::AllowlistRule', + inverse_of: false, dependent: :destroy + has_many :marketplace_access_blocks, class_name: 'Course::Assessment::Marketplace::AccessBlock', + inverse_of: false, dependent: :destroy + # Blocks this user ISSUED. Not `dependent:` anything — destroying them would silently restore + # marketplace access for everyone this admin ever blocked, and `creator_id` is NOT NULL so it + # cannot be nullified either. `reassign_issued_marketplace_blocks` hands authorship to the + # Deleted user instead, which keeps the block standing and satisfies the FK. + has_many :issued_marketplace_access_blocks, class_name: 'Course::Assessment::Marketplace::AccessBlock', + foreign_key: :creator_id, inverse_of: false, + dependent: nil + before_destroy :reassign_issued_marketplace_blocks + accepts_nested_attributes_for :emails scope :ordered_by_name, -> { order(:name) } @@ -96,6 +112,26 @@ def built_in? id == User::SYSTEM_USER_ID || id == User::DELETED_USER_ID end + # Whether the user manages or owns at least one course, in any instance. This is the baseline + # capability for the assessment marketplace: browsing is then further gated by the allow-list. + # `course_users` is not tenant-scoped (CourseUser has no acts_as_tenant), so this correctly + # spans all instances. + # + # @return [Boolean] + def course_manager_or_owner? + course_users.managers.exists? + end + + # Whether the user is an instructor or administrator InstanceUser in ANY instance. This is the + # second baseline capability for the assessment marketplace, a peer of course_manager_or_owner?. + # `instance_users` IS tenant-scoped (acts_as_tenant), so bypass the tenant to span all instances. + # + # @return [Boolean] + def instance_instructor_or_administrator? + ActsAsTenant.without_tenant do + instance_users.where(role: [:instructor, :administrator]).exists? + end + end # Pick the default email and set it as primary email. This method would immediately set the # attributes in the database. # @@ -136,6 +172,14 @@ def build_course_user_from_invitation(invitation) private + # Hands any marketplace blocks this user issued to the Deleted user, so destroying an admin does + # not lift the blocks they put in place (nor trip the NOT NULL FK on `creator_id`). + def reassign_issued_marketplace_blocks + return if id == User::DELETED_USER_ID + + issued_marketplace_access_blocks.update_all(creator_id: User::DELETED_USER_ID) + end + # Gets the default email address record. # # @return [User::Email] The user's primary email address record. diff --git a/app/views/system/admin/marketplace_access/index.json.jbuilder b/app/views/system/admin/marketplace_access/index.json.jbuilder new file mode 100644 index 00000000000..332f37027eb --- /dev/null +++ b/app/views/system/admin/marketplace_access/index.json.jbuilder @@ -0,0 +1,22 @@ +# frozen_string_literal: true +json.users @rows do |row| + json.id row.user.id + json.name row.user.name + json.email row.user.email + json.courseCount row.course_count + json.instanceRole row.instance_role + json.allowedByRules row.allowed_by_rules do |rule| + json.id rule.id + json.ruleType rule.rule_type + json.labelValue marketplace_rule_label_value(rule) + end + json.systemAdmin row.system_admin? + json.blocked row.blocked? + json.blockId row.block_id +end + +json.summary do + json.totalWithAccess @summary[:total_with_access] + json.totalBlocked @summary[:total_blocked] + json.openToEveryone @summary[:open_to_everyone] +end diff --git a/app/views/system/admin/marketplace_allowlist_rules/_rule.json.jbuilder b/app/views/system/admin/marketplace_allowlist_rules/_rule.json.jbuilder new file mode 100644 index 00000000000..d05e2b8ec38 --- /dev/null +++ b/app/views/system/admin/marketplace_allowlist_rules/_rule.json.jbuilder @@ -0,0 +1,9 @@ +# frozen_string_literal: true +json.id rule.id +json.ruleType rule.rule_type +json.userId rule.user_id +json.userName rule.user&.name +json.userEmail rule.user&.email +json.instanceId rule.instance_id +json.instanceName rule.instance&.name +json.emailDomain rule.email_domain diff --git a/app/views/system/admin/marketplace_allowlist_rules/index.json.jbuilder b/app/views/system/admin/marketplace_allowlist_rules/index.json.jbuilder new file mode 100644 index 00000000000..2b07f8a2ad8 --- /dev/null +++ b/app/views/system/admin/marketplace_allowlist_rules/index.json.jbuilder @@ -0,0 +1,5 @@ +# frozen_string_literal: true +json.rules @allowlist_rules do |rule| + json.partial! 'rule', rule: rule +end +json.everyoneRuleId @everyone_rule&.id diff --git a/app/views/system/admin/marketplace_allowlist_rules/preview.json.jbuilder b/app/views/system/admin/marketplace_allowlist_rules/preview.json.jbuilder new file mode 100644 index 00000000000..d6fae59011f --- /dev/null +++ b/app/views/system/admin/marketplace_allowlist_rules/preview.json.jbuilder @@ -0,0 +1,15 @@ +# frozen_string_literal: true +json.matchedCount @summary[:matched_count] +json.newCount @summary[:new_count] +json.blockedCount @summary[:blocked_count] +json.openToEveryone @summary[:open_to_everyone] + +json.users @rows do |row| + json.id row.user.id + json.name row.user.name + json.email row.user.email + json.courseCount row.course_count + json.instanceRole row.instance_role + json.alreadyHasAccess row.already_has_access + json.blocked row.blocked +end \ No newline at end of file diff --git a/config/locales/en/activerecord/attributes.yml b/config/locales/en/activerecord/attributes.yml index f5586333528..8869d328563 100644 --- a/config/locales/en/activerecord/attributes.yml +++ b/config/locales/en/activerecord/attributes.yml @@ -15,6 +15,13 @@ en: weight: 'Order' course/assessment/category/title: default: 'Assessments' + # Admins read these attribute names verbatim: the allow-list controller renders + # `errors.full_messages.to_sentence` straight into a toast, so without these entries a + # validation failure surfaces as the raw i18n key. + course/assessment/marketplace/allowlist_rule: + user_id: 'User' + instance_id: 'Instance' + email_domain: 'Email domain' course/assessment/question: weight: 'Order' course/assessment/submission: diff --git a/config/locales/ko/activerecord/attributes.yml b/config/locales/ko/activerecord/attributes.yml index 6696c1e3611..34ab52f9d39 100644 --- a/config/locales/ko/activerecord/attributes.yml +++ b/config/locales/ko/activerecord/attributes.yml @@ -15,6 +15,10 @@ ko: weight: '순서' course/assessment/category/title: default: '평가' + course/assessment/marketplace/allowlist_rule: + user_id: '사용자' + instance_id: '인스턴스' + email_domain: '이메일 도메인' course/assessment/question: weight: '순서' course/assessment/submission: diff --git a/config/locales/zh/activerecord/attributes.yml b/config/locales/zh/activerecord/attributes.yml index ab0470c80aa..875cd5ba325 100644 --- a/config/locales/zh/activerecord/attributes.yml +++ b/config/locales/zh/activerecord/attributes.yml @@ -15,6 +15,10 @@ zh: weight: '权重' course/assessment/category/title: default: '评估' + course/assessment/marketplace/allowlist_rule: + user_id: '用户' + instance_id: '实例' + email_domain: '电子邮箱域名' course/assessment/question: weight: '权重' course/assessment/submission: diff --git a/config/routes.rb b/config/routes.rb index 026a51fdecf..e40eeb54d47 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -109,6 +109,13 @@ get '/' => 'admin#index' get 'deployment_info' => 'admin#deployment_info' resources :announcements, only: [:index, :create, :update, :destroy] + resources :marketplace_allowlist_rules, only: [:index, :create, :destroy] do + # Dry run: reports who a prospective rule would let in. POST because it carries a body, + # not because it mutates - the action never saves. + post :preview, on: :collection + end + get 'marketplace_access' => 'marketplace_access#index' + resources :marketplace_access_blocks, only: [:create, :destroy] resources :instances, only: [:index, :create, :update, :destroy] resources :users, only: [:index, :update, :destroy] resources :courses, only: [:index, :destroy] diff --git a/db/migrate/20260720154800_create_course_assessment_marketplace_allowlist_rules.rb b/db/migrate/20260720154800_create_course_assessment_marketplace_allowlist_rules.rb new file mode 100644 index 00000000000..91e58838c79 --- /dev/null +++ b/db/migrate/20260720154800_create_course_assessment_marketplace_allowlist_rules.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true +class CreateCourseAssessmentMarketplaceAllowlistRules < ActiveRecord::Migration[7.2] + def change + create_table :course_assessment_marketplace_allowlist_rules do |t| + t.integer :rule_type, null: false + + t.references :user, + foreign_key: { to_table: :users }, + null: true, + index: true + + t.references :instance, + foreign_key: true, + null: true, + index: true + + t.string :email_domain, null: true + + t.timestamps + end + + add_index :course_assessment_marketplace_allowlist_rules, + :email_domain + + add_index :course_assessment_marketplace_allowlist_rules, + :user_id, + unique: true, + where: "rule_type = 0", + name: "index_marketplace_allowlist_rules_one_per_user" + + add_index :course_assessment_marketplace_allowlist_rules, + :instance_id, + unique: true, + where: "rule_type = 1", + name: "index_marketplace_allowlist_rules_one_per_instance" + + add_index :course_assessment_marketplace_allowlist_rules, + :email_domain, + unique: true, + where: "rule_type = 2", + name: "index_marketplace_allowlist_rules_one_per_email_domain" + + add_index :course_assessment_marketplace_allowlist_rules, + :rule_type, + unique: true, + where: "rule_type = 3", + name: "index_marketplace_allowlist_rules_one_everyone" + + create_table :course_assessment_marketplace_access_blocks do |t| + t.references :user, null: false, foreign_key: true, index: { unique: true } + t.references :creator, null: false, foreign_key: { to_table: :users } + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 7eeadc353c1..0d63cf08be9 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2026_07_07_000002) do +ActiveRecord::Schema[7.2].define(version: 2026_07_20_154800) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" enable_extension "uuid-ossp" @@ -270,6 +270,15 @@ t.index ["question_id"], name: "index_course_assessment_live_feedbacks_on_question_id" end + create_table "course_assessment_marketplace_access_blocks", force: :cascade do |t| + t.bigint "user_id", null: false + t.bigint "creator_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["creator_id"], name: "idx_on_creator_id_becaf2e041" + t.index ["user_id"], name: "index_course_assessment_marketplace_access_blocks_on_user_id", unique: true + end + create_table "course_assessment_marketplace_adoptions", force: :cascade do |t| t.bigint "listing_id", null: false t.bigint "destination_course_id", null: false @@ -286,6 +295,22 @@ t.index ["updater_id"], name: "fk__cama_updater_id" end + create_table "course_assessment_marketplace_allowlist_rules", force: :cascade do |t| + t.integer "rule_type", null: false + t.bigint "user_id" + t.bigint "instance_id" + t.string "email_domain" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["email_domain"], name: "idx_on_email_domain_6577b88d4e" + t.index ["email_domain"], name: "index_marketplace_allowlist_rules_one_per_email_domain", unique: true, where: "(rule_type = 2)" + t.index ["instance_id"], name: "idx_on_instance_id_77af5cff27" + t.index ["instance_id"], name: "index_marketplace_allowlist_rules_one_per_instance", unique: true, where: "(rule_type = 1)" + t.index ["rule_type"], name: "index_marketplace_allowlist_rules_one_everyone", unique: true, where: "(rule_type = 3)" + t.index ["user_id"], name: "index_course_assessment_marketplace_allowlist_rules_on_user_id" + t.index ["user_id"], name: "index_marketplace_allowlist_rules_one_per_user", unique: true, where: "(rule_type = 0)" + end + create_table "course_assessment_marketplace_listings", force: :cascade do |t| t.bigint "assessment_id", null: false t.boolean "published", default: false, null: false @@ -1978,11 +2003,15 @@ add_foreign_key "course_assessment_live_feedbacks", "course_assessment_questions", column: "question_id" add_foreign_key "course_assessment_live_feedbacks", "course_assessments", column: "assessment_id" add_foreign_key "course_assessment_live_feedbacks", "users", column: "creator_id" + add_foreign_key "course_assessment_marketplace_access_blocks", "users" + add_foreign_key "course_assessment_marketplace_access_blocks", "users", column: "creator_id" add_foreign_key "course_assessment_marketplace_adoptions", "course_assessment_marketplace_listings", column: "listing_id", name: "fk_course_assessment_marketplace_adoptions_listing_id", on_delete: :cascade add_foreign_key "course_assessment_marketplace_adoptions", "course_assessments", column: "duplicated_assessment_id", name: "fk_cama_duplicated_assessment_id", on_delete: :cascade add_foreign_key "course_assessment_marketplace_adoptions", "courses", column: "destination_course_id", name: "fk_cama_destination_course_id", on_delete: :cascade add_foreign_key "course_assessment_marketplace_adoptions", "users", column: "creator_id", name: "fk_course_assessment_marketplace_adoptions_creator_id" add_foreign_key "course_assessment_marketplace_adoptions", "users", column: "updater_id", name: "fk_course_assessment_marketplace_adoptions_updater_id" + add_foreign_key "course_assessment_marketplace_allowlist_rules", "instances" + add_foreign_key "course_assessment_marketplace_allowlist_rules", "users" add_foreign_key "course_assessment_marketplace_listings", "course_assessments", column: "assessment_id", name: "fk_course_assessment_marketplace_listings_assessment_id", on_delete: :cascade add_foreign_key "course_assessment_marketplace_listings", "users", column: "creator_id", name: "fk_course_assessment_marketplace_listings_creator_id" add_foreign_key "course_assessment_marketplace_listings", "users", column: "publisher_id", name: "fk_course_assessment_marketplace_listings_publisher_id" diff --git a/spec/controllers/course/assessment/marketplace/listings_controller_spec.rb b/spec/controllers/course/assessment/marketplace/listings_controller_spec.rb index 9c02a8ac2db..bdf1398de0e 100644 --- a/spec/controllers/course/assessment/marketplace/listings_controller_spec.rb +++ b/spec/controllers/course/assessment/marketplace/listings_controller_spec.rb @@ -12,6 +12,8 @@ before { controller_sign_in(controller, manager.user) } describe 'GET #index' do + before { create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: manager.user) } + let!(:published) { create(:course_assessment_marketplace_listing, published: true) } let!(:unpublished) { create(:course_assessment_marketplace_listing, published: false) } @@ -69,12 +71,117 @@ end end end + describe 'GET #index visibility gate' do + subject { get :index, params: { course_id: course.id, format: :json } } + + # The suite runs with `use_transactional_fixtures = false` (see spec/rails_helper.rb), so rows + # persist across examples/runs. `:everyone` is a DB-enforced singleton (one row allowed), so any + # leftover row here would either block a later `create(:everyone)` with a uniqueness error or + # spuriously widen access in a sibling example. Mirrors the cleanup in + # spec/models/course/assessment/marketplace/allowlist_rule_spec.rb. + before { Course::Assessment::Marketplace::AllowlistRule.delete_all } + + context 'when the manager is not on the allow-list' do + before { controller_sign_in(controller, manager.user) } + + it 'denies access' do + expect { subject }.to raise_exception(CanCan::AccessDenied) + end + end + + context 'when an allow-list rule matches the manager' do + before do + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: manager.user) + controller_sign_in(controller, manager.user) + end + + it 'permits access' do + expect { subject }.not_to raise_exception + end + end + + context 'when the user is a system administrator' do + let(:admin) { create(:administrator) } + before do + create(:course_manager, course: course, user: admin) + controller_sign_in(controller, admin) + end + + it 'permits access without an allow-list rule' do + expect { subject }.not_to raise_exception + end + end + + context "when an 'everyone' rule exists" do + before do + create(:course_assessment_marketplace_allowlist_rule, :everyone) + controller_sign_in(controller, manager.user) + end + + it 'permits a manager who has no matching scoped rule' do + expect { subject }.not_to raise_exception + end + end + + context "when an 'everyone' rule exists but the user is a student" do + let(:student) { create(:course_student, course: course).user } + before do + create(:course_assessment_marketplace_allowlist_rule, :everyone) + controller_sign_in(controller, student) + end + + it 'still denies a non-manager (the manager gate holds)' do + expect { subject }.to raise_exception(CanCan::AccessDenied) + end + end + + context 'when the user is an observer here but manages another course' do + let(:roamer) { create(:course_observer, course: course).user } + before do + create(:course_manager, course: create(:course), user: roamer) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: roamer) + controller_sign_in(controller, roamer) + end + + it 'permits browsing (access is per-person, not per-current-course role)' do + expect { subject }.not_to raise_exception + end + end + + context 'when the user manages another course but is not on the allow-list' do + let(:roamer) { create(:course_observer, course: course).user } + before do + create(:course_manager, course: create(:course), user: roamer) + controller_sign_in(controller, roamer) + end + + it 'denies access (allow-list is still required even for a manager elsewhere)' do + expect { subject }.to raise_exception(CanCan::AccessDenied) + end + end + + context 'when an allow-listed user manages no course' do + let(:pupil) { create(:course_student, course: course).user } + before do + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: pupil) + controller_sign_in(controller, pupil) + end + + it 'denies access (must manage at least one course)' do + expect { subject }.to raise_exception(CanCan::AccessDenied) + end + end + + end + describe 'POST #duplicate' do # `have_enqueued_job` requires the :test adapter; the test env defaults to :background_thread. # `run_rescue` re-enables handle_access_denied so AccessDenied renders 403 rather than # propagating (controller specs bypass_rescue by default — see spec/support/controller_exceptions.rb). run_rescue + before { create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: manager.user) } + with_active_job_queue_adapter(:test) do let!(:listing) { create(:course_assessment_marketplace_listing, published: true) } let!(:tab) { course.assessment_categories.first.tabs.first } @@ -116,6 +223,8 @@ # propagating (controller specs bypass_rescue by default — see spec/support/controller_exceptions.rb). run_rescue + before { create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: manager.user) } + let!(:listing) do assessment = create(:assessment, course: create(:course)) create(:course_assessment_question_multiple_response, :multiple_choice, assessment: assessment) @@ -175,6 +284,7 @@ ActsAsTenant.with_tenant(home_instance) do course = create(:course) manager = create(:course_manager, course: course) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: manager.user) controller_sign_in(controller, manager.user) # Point the request at the home instance's host so `deduce_tenant` resolves it (this # describe is outside `with_tenant`, which would otherwise set the host header for us). diff --git a/spec/controllers/course/assessment/marketplace/questions_controller_spec.rb b/spec/controllers/course/assessment/marketplace/questions_controller_spec.rb index b6d35b3839c..de6d5949df3 100644 --- a/spec/controllers/course/assessment/marketplace/questions_controller_spec.rb +++ b/spec/controllers/course/assessment/marketplace/questions_controller_spec.rb @@ -35,7 +35,10 @@ let(:destination_course) { create(:course) } let(:manager) { create(:course_manager, course: destination_course).user } - before { controller_sign_in(controller, manager) } + before do + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: manager) + controller_sign_in(controller, manager) + end it 'serializes the question across instances' do get :show, as: :json, params: { diff --git a/spec/controllers/course/assessment_marketplace_component_spec.rb b/spec/controllers/course/assessment_marketplace_component_spec.rb index 6fa1c1f6aab..78cd6db53f5 100644 --- a/spec/controllers/course/assessment_marketplace_component_spec.rb +++ b/spec/controllers/course/assessment_marketplace_component_spec.rb @@ -15,7 +15,10 @@ context 'when the user can access the marketplace (course manager)' do let(:user) { create(:course_manager, course: course).user } - before { controller_sign_in(controller, user) } + before do + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: user) + controller_sign_in(controller, user) + end it 'exposes an admin sidebar item pointing at the marketplace' do item = subject.sidebar_items.find { |i| i[:key] == :admin_marketplace } diff --git a/spec/controllers/system/admin/marketplace_access_blocks_controller_spec.rb b/spec/controllers/system/admin/marketplace_access_blocks_controller_spec.rb new file mode 100644 index 00000000000..b320410020f --- /dev/null +++ b/spec/controllers/system/admin/marketplace_access_blocks_controller_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe System::Admin::MarketplaceAccessBlocksController, type: :controller do + let!(:instance) { Instance.default } + + with_tenant(:instance) do + let(:admin) { create(:administrator) } + before do + Course::Assessment::Marketplace::AccessBlock.delete_all + controller_sign_in(controller, admin) + end + + describe 'POST #create' do + it 'blocks the user and returns the block id' do + target = create(:user) + expect do + post :create, format: :json, params: { user_id: target.id } + end.to change { Course::Assessment::Marketplace::AccessBlock.count }.by(1) + expect(response).to have_http_status(:ok) + expect(response.parsed_body['userId']).to eq(target.id) + expect(response.parsed_body['id']).to be_present + end + + it 'rejects a duplicate block for the same user' do + target = create(:user) + create(:course_assessment_marketplace_access_block, user: target) + expect do + post :create, format: :json, params: { user_id: target.id } + end.not_to(change { Course::Assessment::Marketplace::AccessBlock.count }) + expect(response).to have_http_status(:bad_request) + end + end + + describe 'DELETE #destroy' do + it 'removes the block' do + block = create(:course_assessment_marketplace_access_block) + expect do + delete :destroy, format: :json, params: { id: block.id } + end.to change { Course::Assessment::Marketplace::AccessBlock.count }.by(-1) + expect(response).to have_http_status(:ok) + end + end + + describe 'authorization' do + run_rescue + + it 'forbids a non-administrator' do + controller_sign_in(controller, create(:user)) + post :create, format: :json, params: { user_id: create(:user).id } + expect(response).to have_http_status(:forbidden) + end + end + end +end diff --git a/spec/controllers/system/admin/marketplace_access_controller_spec.rb b/spec/controllers/system/admin/marketplace_access_controller_spec.rb new file mode 100644 index 00000000000..8d753cfaf6c --- /dev/null +++ b/spec/controllers/system/admin/marketplace_access_controller_spec.rb @@ -0,0 +1,129 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe System::Admin::MarketplaceAccessController, type: :controller do + let!(:instance) { Instance.default } + + with_tenant(:instance) do + let(:admin) { create(:administrator) } + before do + Course::Assessment::Marketplace::AllowlistRule.delete_all + Course::Assessment::Marketplace::AccessBlock.delete_all + # LOWER() and no '@' anchor: the uniqueness index is on `lower(email)` while SQL LIKE is + # case-sensitive, so an anchored, case-sensitive pattern leaves rows behind that collide on + # the next run. + User::Email.where('LOWER(email) LIKE ?', '%schools.gov.sg').delete_all + controller_sign_in(controller, admin) + end + + describe 'GET #index' do + render_views + + it 'lists eligible users with annotations and a summary' do + manager = create(:course_manager, course: create(:course)) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: manager.user) + + get :index, format: :json + expect(response).to have_http_status(:ok) + + row = response.parsed_body['users'].find { |u| u['id'] == manager.user.id } + expect(row).to be_present + expect(row['allowedByRules'].map { |r| r['ruleType'] }).to eq(['user']) + expect(row['courseCount']).to eq(1) + expect(row['blocked']).to be(false) + expect(row['systemAdmin']).to be(false) + # System admins are always listed and always count as having access; the test DB accumulates + # them across runs (nothing rolls back), so the total is relative to however many exist. + expect(response.parsed_body['summary']['totalWithAccess']).to eq(1 + User.administrator.count) + expect(response.parsed_body['summary']['openToEveryone']).to be(false) + end + + it 'flags a blocked user with a blockId' do + manager = create(:course_manager, course: create(:course)) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: manager.user) + block = create(:course_assessment_marketplace_access_block, user: manager.user) + + get :index, format: :json + row = response.parsed_body['users'].find { |u| u['id'] == manager.user.id } + expect(row['blocked']).to be(true) + expect(row['blockId']).to eq(block.id) + expect(response.parsed_body['summary']['totalWithAccess']).to eq(User.administrator.count) + end + end + + describe 'authorization' do + run_rescue + + it 'forbids a non-administrator' do + controller_sign_in(controller, create(:user)) + get :index, format: :json + expect(response).to have_http_status(:forbidden) + end + end + describe 'GET #index serialization' do + render_views + + it 'serializes every matching rule with its label, and the blocked total' do + user = create(:user, email: 'listed@schools.gov.sg') + create(:course_manager, course: create(:course), user: user) + user_rule = create(:course_assessment_marketplace_allowlist_rule, + rule_type: :user, user: user) + domain_rule = create(:course_assessment_marketplace_allowlist_rule, :for_email_domain, + email_domain: 'schools.gov.sg') + create(:course_assessment_marketplace_access_block, user: user) + + get :index, format: :json + + expect(response).to have_http_status(:ok) + row = response.parsed_body['users'].find { |u| u['id'] == user.id } + expect(row).not_to be_nil + expect(row['allowedByRules']).to contain_exactly( + { 'id' => user_rule.id, 'ruleType' => 'user', 'labelValue' => user.name }, + { 'id' => domain_rule.id, 'ruleType' => 'email_domain', + 'labelValue' => 'schools.gov.sg' } + ) + expect(response.parsed_body['summary']['totalBlocked']).to eq(1) + end + + it 'serializes an instance rule with the instance name as its label' do + other_instance = create(:instance) + user = create(:user) + ActsAsTenant.with_tenant(other_instance) do + create(:instance_user, :instructor, user: user, instance: other_instance) + end + rule = create(:course_assessment_marketplace_allowlist_rule, + rule_type: :instance, instance: other_instance) + + get :index, format: :json + + row = response.parsed_body['users'].find { |u| u['id'] == user.id } + expect(row['allowedByRules']).to eq( + ['id' => rule.id, 'ruleType' => 'instance', 'labelValue' => other_instance.name] + ) + end + + it 'serializes an empty rule list for a user listed only because they are blocked' do + cu = create(:course_manager, course: create(:course)) + create(:course_assessment_marketplace_access_block, user: cu.user) + + get :index, format: :json + + row = response.parsed_body['users'].find { |u| u['id'] == cu.user.id } + expect(row['allowedByRules']).to eq([]) + expect(row['blocked']).to be(true) + end + + it 'serializes a system admin who manages nothing and matches no rule' do + admin = create(:administrator) + + get :index, format: :json + + row = response.parsed_body['users'].find { |u| u['id'] == admin.id } + expect(row).not_to be_nil + expect(row['systemAdmin']).to be(true) + expect(row['allowedByRules']).to eq([]) + expect(row['courseCount']).to eq(0) + end + end + end +end diff --git a/spec/controllers/system/admin/marketplace_allowlist_rules_controller_spec.rb b/spec/controllers/system/admin/marketplace_allowlist_rules_controller_spec.rb new file mode 100644 index 00000000000..4ef36f310c3 --- /dev/null +++ b/spec/controllers/system/admin/marketplace_allowlist_rules_controller_spec.rb @@ -0,0 +1,291 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe System::Admin::MarketplaceAllowlistRulesController, type: :controller do + let!(:instance) { Instance.default } + + with_tenant(:instance) do + let(:admin) { create(:administrator) } + before { controller_sign_in(controller, admin) } + + describe 'POST #create' do + # Email-domain rules are unique per domain and specs commit, so the row this example creates + # would collide with itself on the next run. Clear it first. + before do + Course::Assessment::Marketplace::AllowlistRule. + rule_type_email_domain.where(email_domain: 'schools.gov.sg').delete_all + end + + subject do + post :create, format: :json, params: { + allowlist_rule: { rule_type: 'email_domain', email_domain: 'schools.gov.sg' } + } + end + + it 'creates an email-domain rule' do + expect { subject }. + to change { Course::Assessment::Marketplace::AllowlistRule.count }.by(1) + expect(response).to have_http_status(:ok) + end + end + + describe 'POST #create for a user rule by email' do + render_views + # No transactional fixtures / DatabaseCleaner here (see GET #index note), so a user with this + # hardcoded email can persist from an earlier run and collide on email uniqueness. Clear it. + before { User::Email.where(email: 'teacher@school.edu').delete_all } + + it 'resolves a confirmed email to the owning user and creates a user rule' do + target = create(:user, email: 'teacher@school.edu') + expect do + post :create, format: :json, params: { + allowlist_rule: { rule_type: 'user', email: 'teacher@school.edu' } + } + end.to change { Course::Assessment::Marketplace::AllowlistRule.rule_type_user.count }.by(1) + expect(response).to have_http_status(:ok) + expect(Course::Assessment::Marketplace::AllowlistRule.rule_type_user.last.user).to eq(target) + end + + it 'serializes the resolved user\'s email as userEmail in the rendered rule' do + create(:user, email: 'teacher@school.edu') + post :create, format: :json, params: { + allowlist_rule: { rule_type: 'user', email: 'teacher@school.edu' } + } + + expect(response).to have_http_status(:ok) + expect(response.parsed_body['userEmail']).to eq('teacher@school.edu') + end + + it 'rejects an email that matches no user' do + expect do + post :create, format: :json, params: { + allowlist_rule: { rule_type: 'user', email: 'nobody@nowhere.test' } + } + end.not_to(change { Course::Assessment::Marketplace::AllowlistRule.count }) + expect(response).to have_http_status(:bad_request) + expect(response.parsed_body['errors']).to include('No user with that email.') + end + end + + describe 'GET #index' do + render_views + # This suite runs with `use_transactional_fixtures = false` and no DatabaseCleaner, so rows + # created by earlier local runs of this factory persist in the dev/test DB; scope to a clean + # slate here so the size assertion below is deterministic. + before do + Course::Assessment::Marketplace::AllowlistRule.delete_all + create(:course_assessment_marketplace_allowlist_rule, :for_email_domain) + end + + it 'lists the rules' do + get :index, format: :json + expect(response).to have_http_status(:ok) + expect(response.parsed_body['rules'].size).to eq(1) + end + end + + describe 'GET #index everyone-mode reporting' do + render_views + before do + Course::Assessment::Marketplace::AllowlistRule.delete_all + create(:course_assessment_marketplace_allowlist_rule, :for_email_domain) + end + + it 'reports everyoneRuleId null and lists only scoped rules when no everyone rule exists' do + get :index, format: :json + expect(response).to have_http_status(:ok) + expect(response.parsed_body['everyoneRuleId']).to be_nil + expect(response.parsed_body['rules'].size).to eq(1) + end + + it 'reports everyoneRuleId and excludes the everyone rule from the list' do + everyone = create(:course_assessment_marketplace_allowlist_rule, :everyone) + get :index, format: :json + expect(response).to have_http_status(:ok) + expect(response.parsed_body['everyoneRuleId']).to eq(everyone.id) + expect(response.parsed_body['rules'].map { |r| r['ruleType'] }).not_to include('everyone') + expect(response.parsed_body['rules'].size).to eq(1) + end + end + + describe "POST #create with rule_type 'everyone'" do + before { Course::Assessment::Marketplace::AllowlistRule.delete_all } + + it 'opens the marketplace to everyone' do + expect do + post :create, format: :json, params: { allowlist_rule: { rule_type: 'everyone' } } + end.to change { Course::Assessment::Marketplace::AllowlistRule.rule_type_everyone.count }.by(1) + expect(response).to have_http_status(:ok) + end + + it 'rejects a second everyone rule' do + create(:course_assessment_marketplace_allowlist_rule, :everyone) + expect do + post :create, format: :json, params: { allowlist_rule: { rule_type: 'everyone' } } + end.not_to(change { Course::Assessment::Marketplace::AllowlistRule.count }) + expect(response).to have_http_status(:bad_request) + end + + it 'surfaces the uniqueness error when rejected' do + create(:course_assessment_marketplace_allowlist_rule, :everyone) + post :create, format: :json, params: { allowlist_rule: { rule_type: 'everyone' } } + expect(response.parsed_body['errors']).to include('already been taken') + end + end + + describe 'DELETE #destroy' do + let!(:rule) { create(:course_assessment_marketplace_allowlist_rule, :for_email_domain) } + + it 'removes the rule' do + expect { delete :destroy, format: :json, params: { id: rule.id } }. + to change { Course::Assessment::Marketplace::AllowlistRule.count }.by(-1) + expect(response).to have_http_status(:ok) + end + end + + describe 'authorization' do + run_rescue + + it 'forbids a non-administrator' do + controller_sign_in(controller, create(:user)) + get :index, format: :json + expect(response).to have_http_status(:forbidden) + end + end + + describe 'POST #preview' do + render_views + + before do + Course::Assessment::Marketplace::AllowlistRule.delete_all + Course::Assessment::Marketplace::AccessBlock.delete_all + # LOWER() and no '@' anchor: the uniqueness index is on `lower(email)` while SQL LIKE is + # case-sensitive, so an anchored, case-sensitive pattern misses rows the index will still + # collide on. + User::Email.where('LOWER(email) LIKE ?', '%preview.test').delete_all + end + + def preview(params) + post :preview, format: :json, params: { allowlist_rule: params } + end + + it 'counts eligible staff a domain rule would match, and how many are new' do + newcomer = create(:user, email: 'newcomer@preview.test') + create(:course_manager, course: create(:course), user: newcomer) + existing = create(:user, email: 'existing@preview.test') + create(:course_manager, course: create(:course), user: existing) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: existing) + + preview(rule_type: 'email_domain', email_domain: 'preview.test') + + expect(response).to have_http_status(:ok) + body = response.parsed_body + expect(body['matchedCount']).to eq(2) + expect(body['newCount']).to eq(1) + expect(body['blockedCount']).to eq(0) + expect(body['openToEveryone']).to be(false) + expect(body['users'].map { |u| u['id'] }).to contain_exactly(newcomer.id, existing.id) + expect(body['users'].find { |u| u['id'] == existing.id }['alreadyHasAccess']).to be(true) + expect(body['users'].find { |u| u['id'] == newcomer.id }['alreadyHasAccess']).to be(false) + end + + it 'persists nothing' do + create(:course_manager, course: create(:course), + user: create(:user, email: 'dryrun@preview.test')) + + expect { preview(rule_type: 'email_domain', email_domain: 'preview.test') }. + not_to(change { Course::Assessment::Marketplace::AllowlistRule.count }) + end + + it 'excludes users who are not baseline-eligible' do + create(:course_student, course: create(:course), + user: create(:user, email: 'student@preview.test')) + + preview(rule_type: 'email_domain', email_domain: 'preview.test') + + expect(response.parsed_body['matchedCount']).to eq(0) + end + + it 'counts a blocked match but never as new' do + blocked = create(:user, email: 'blocked@preview.test') + create(:course_manager, course: create(:course), user: blocked) + create(:course_assessment_marketplace_access_block, user: blocked) + + preview(rule_type: 'email_domain', email_domain: 'preview.test') + + body = response.parsed_body + expect(body['matchedCount']).to eq(1) + expect(body['newCount']).to eq(0) + # Counted separately from the already-has-access remainder: the UI names the two groups + # apart, and a blocked user is held back by their own block, not by prior access. + expect(body['blockedCount']).to eq(1) + expect(body['users'].first['blocked']).to be(true) + end + + it 'lists blocked, then already-cleared, then newly granted matches' do + # Named so the alphabetical order the query starts from is the exact REVERSE of the + # expected one; without the grouping this example would still pass on a name-sorted list. + blocked = create(:user, name: 'Zoe Blocked', email: 'zoe@preview.test') + create(:course_manager, course: create(:course), user: blocked) + create(:course_assessment_marketplace_access_block, user: blocked) + existing = create(:user, name: 'Mabel Existing', email: 'mabel@preview.test') + create(:course_manager, course: create(:course), user: existing) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: existing) + newcomer = create(:user, name: 'Adam New', email: 'adam@preview.test') + create(:course_manager, course: create(:course), user: newcomer) + + preview(rule_type: 'email_domain', email_domain: 'preview.test') + + expect(response.parsed_body['users'].map { |u| u['id'] }). + to eq([blocked.id, existing.id, newcomer.id]) + end + + it 'reports zero new when the marketplace is already open to everyone' do + create(:course_manager, course: create(:course), + user: create(:user, email: 'open@preview.test')) + create(:course_assessment_marketplace_allowlist_rule, :everyone) + + preview(rule_type: 'email_domain', email_domain: 'preview.test') + + body = response.parsed_body + expect(body['openToEveryone']).to be(true) + expect(body['matchedCount']).to eq(1) + expect(body['newCount']).to eq(0) + end + + it 'returns zero matches for a user rule whose target is not eligible' do + create(:user, email: 'nobody@preview.test') # manages nothing + + preview(rule_type: 'user', email: 'nobody@preview.test') + + expect(response).to have_http_status(:ok) + expect(response.parsed_body['matchedCount']).to eq(0) + end + + it 'rejects an email matching no user' do + preview(rule_type: 'user', email: 'ghost@preview.test') + + expect(response).to have_http_status(:bad_request) + expect(response.parsed_body['errors']).to include('No user with that email.') + end + + it 'rejects a rule that already exists' do + create(:course_assessment_marketplace_allowlist_rule, + rule_type: :email_domain, email_domain: 'preview.test') + + preview(rule_type: 'email_domain', email_domain: 'preview.test') + + expect(response).to have_http_status(:bad_request) + # Attribute name omitted: StubbedI18nBackend returns the raw key for + # `activerecord.attributes.*`, so full_messages can never render "Email domain" here. + expect(response.parsed_body['errors']).to include('already has a rule.') + end + + it 'denies a non-administrator' do + controller_sign_in(controller, create(:user)) + expect { preview(rule_type: 'email_domain', email_domain: 'preview.test') }. + to raise_exception(CanCan::AccessDenied) + end + end + end +end diff --git a/spec/factories/course_assessment_marketplace_access_blocks.rb b/spec/factories/course_assessment_marketplace_access_blocks.rb new file mode 100644 index 00000000000..471edf6f26a --- /dev/null +++ b/spec/factories/course_assessment_marketplace_access_blocks.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true +FactoryBot.define do + factory :course_assessment_marketplace_access_block, + class: 'Course::Assessment::Marketplace::AccessBlock' do + association :user + association :creator, factory: :user + end +end diff --git a/spec/factories/course_assessment_marketplace_allowlist_rules.rb b/spec/factories/course_assessment_marketplace_allowlist_rules.rb new file mode 100644 index 00000000000..ae96fb09113 --- /dev/null +++ b/spec/factories/course_assessment_marketplace_allowlist_rules.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true +FactoryBot.define do + factory :course_assessment_marketplace_allowlist_rule, + class: 'Course::Assessment::Marketplace::AllowlistRule' do + # Default to a self-contained email-domain rule so the bare factory is valid under + # `factory_bot:lint`. Traits below override `rule_type` (and supply any needed association). + rule_type { :email_domain } + # Unique per invocation. Specs commit (use_transactional_fixtures is false), and email-domain + # rules are now unique per domain, so a hardcoded default would collide with the row committed + # by the previous run. + sequence(:email_domain) { |n| "domain-#{n}-#{SecureRandom.hex(3)}.test" } + + trait :for_user do + rule_type { :user } + association :user + end + trait :for_instance do + rule_type { :instance } + association :instance + end + trait :for_email_domain do + rule_type { :email_domain } + end + trait :everyone do + rule_type { :everyone } + end + end +end diff --git a/spec/models/course/assessment/marketplace/access_block_spec.rb b/spec/models/course/assessment/marketplace/access_block_spec.rb new file mode 100644 index 00000000000..bb97b9405ee --- /dev/null +++ b/spec/models/course/assessment/marketplace/access_block_spec.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe Course::Assessment::Marketplace::AccessBlock, type: :model do + let!(:instance) { Instance.default } + + before { described_class.delete_all } + + with_tenant(:instance) do + describe 'validations' do + it 'is valid with a user and creator' do + block = build(:course_assessment_marketplace_access_block) + expect(block).to be_valid + end + + it 'rejects a second block for the same user' do + user = create(:user) + create(:course_assessment_marketplace_access_block, user: user) + duplicate = build(:course_assessment_marketplace_access_block, user: user) + expect(duplicate).not_to be_valid + expect(duplicate.errors[:user_id]).to be_present + end + end + + describe '.blocked?' do + it 'is false for a nil user' do + expect(described_class.blocked?(nil)).to be(false) + end + + it 'is false when the user has no block' do + expect(described_class.blocked?(create(:user))).to be(false) + end + + it 'is true when the user has a block' do + user = create(:user) + create(:course_assessment_marketplace_access_block, user: user) + expect(described_class.blocked?(user)).to be(true) + end + end + + describe '.blocked_user_ids' do + it 'returns the user ids of all blocks' do + user = create(:user) + create(:course_assessment_marketplace_access_block, user: user) + expect(described_class.blocked_user_ids).to contain_exactly(user.id) + end + end + + describe 'when the admin who issued the block is destroyed' do + # `creator_id` is NOT NULL and FKs to users, so this raised PG::ForeignKeyViolation. The block + # must survive — it is a decision about the BLOCKED person, not about its author — so + # authorship is reassigned to the Deleted user rather than the row being destroyed. + it 'keeps the block and reassigns it to the Deleted user' do + creator = create(:administrator) + block = create(:course_assessment_marketplace_access_block, creator: creator) + + expect { ActsAsTenant.without_tenant { creator.destroy } }. + not_to(change { described_class.count }) + expect(block.reload.creator_id).to eq(User::DELETED_USER_ID) + end + end + + describe 'when the blocked user is destroyed' do + # The blocks table has an FK to users with no ON DELETE, so without a `dependent:` association + # on User the admin panel's delete-user action dies with PG::ForeignKeyViolation. + it 'destroys the block instead of raising a foreign-key violation' do + user = create(:user) + create(:course_assessment_marketplace_access_block, user: user) + + expect { ActsAsTenant.without_tenant { user.destroy } }. + to change { described_class.count }.by(-1) + end + end + end +end diff --git a/spec/models/course/assessment/marketplace/access_list_query_spec.rb b/spec/models/course/assessment/marketplace/access_list_query_spec.rb new file mode 100644 index 00000000000..ddabc899bc8 --- /dev/null +++ b/spec/models/course/assessment/marketplace/access_list_query_spec.rb @@ -0,0 +1,258 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe Course::Assessment::Marketplace::AccessListQuery, type: :model do + let!(:instance) { Instance.default } + + # Specs here commit (use_transactional_fixtures is false repo-wide), so rows from previous runs + # persist. User::Email additionally enforces uniqueness, so the allow-listed-domain addresses below + # must be cleared too or re-creating them raises RecordInvalid. + before do + Course::Assessment::Marketplace::AllowlistRule.delete_all + Course::Assessment::Marketplace::AccessBlock.delete_all + # LOWER() and no '@' anchor: the uniqueness index is on `lower(email)` while SQL LIKE is + # case-sensitive, so an anchored, case-sensitive pattern silently leaves rows behind that + # collide on the next run. + User::Email.where('LOWER(email) LIKE ?', '%schools.gov.sg').delete_all + end + + with_tenant(:instance) do + it 'excludes a baseline user when no rule matches them' do + create(:course_manager, course: create(:course)) # manager, but no allow-list rule + # System admins are listed unconditionally (they bypass every gate), and the test DB always + # holds at least the seeded one — so this asserts on the non-admin rows. + expect(described_class.new.rows.reject(&:system_admin?)).to be_empty + end + + it 'includes a manager cleared by a user rule, annotated with course count and rule' do + cu = create(:course_manager, course: create(:course)) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: cu.user) + + row = described_class.new.rows.find { |r| r.user == cu.user } + expect(row).not_to be_nil + expect(row.course_count).to eq(1) + expect(row.instance_role).to be_nil + expect(row.allowed_by_rules.map(&:rule_type)).to eq(['user']) + expect(row.blocked?).to be(false) + end + + it 'includes an instance instructor (managing no course) under an everyone rule' do + user = create(:user) + other_instance = create(:instance) + ActsAsTenant.with_tenant(other_instance) do + create(:instance_user, :instructor, user: user, instance: other_instance) + end + create(:course_assessment_marketplace_allowlist_rule, :everyone) + + row = described_class.new.rows.find { |r| r.user == user } + expect(row).not_to be_nil + expect(row.course_count).to eq(0) + expect(row.instance_role).to eq('instructor') + # An everyone rule is a page-level mode, not a per-row reason: rows carry no scoped rules. + expect(row.allowed_by_rules).to be_empty + end + + it 'includes a manager cleared by an email-domain rule' do + user = create(:user, email: 'teacher@schools.gov.sg') + create(:course_manager, course: create(:course), user: user) + create(:course_assessment_marketplace_allowlist_rule, :for_email_domain, + email_domain: 'schools.gov.sg') + + row = described_class.new.rows.find { |r| r.user == user } + expect(row).not_to be_nil + expect(row.allowed_by_rules.map(&:rule_type)).to eq(['email_domain']) + end + + it 'excludes a manager whose only allow-listed-domain email is unconfirmed' do + user = create(:user) # has a confirmed primary email at a non-matching domain + create(:course_manager, course: create(:course), user: user) + create(:user_email, :unconfirmed, email: 'pending@schools.gov.sg', + user: user, primary: false) + create(:course_assessment_marketplace_allowlist_rule, :for_email_domain, + email_domain: 'schools.gov.sg') + + expect(described_class.new.rows.map(&:user)).not_to include(user) + end + + it 'includes a manager cleared by an instance rule' do + cu = create(:course_manager, course: create(:course)) + # cu.user has a normal InstanceUser in the default instance via the after_create callback, + # so an instance rule for the default instance clears them. + create(:course_assessment_marketplace_allowlist_rule, rule_type: :instance, instance: instance) + + row = described_class.new.rows.find { |r| r.user == cu.user } + expect(row).not_to be_nil + expect(row.allowed_by_rules.map(&:rule_type)).to eq(['instance']) + end + + it 'does not include a non-baseline user even when a user rule targets them' do + cu = create(:course_student, course: create(:course)) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: cu.user) + expect(described_class.new.rows.map(&:user)).not_to include(cu.user) + end + + it 'keeps a blocked user in the list, flagged with the block id' do + cu = create(:course_manager, course: create(:course)) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: cu.user) + block = create(:course_assessment_marketplace_access_block, user: cu.user) + + row = described_class.new.rows.find { |r| r.user == cu.user } + expect(row.blocked?).to be(true) + expect(row.block_id).to eq(block.id) + end + + # Without this, dropping the `instance_id` filter from RuleMatchQuery#matched_instance_members + # would still pass every other example — the instance-rule test above uses only one instance. + it 'does not clear a user via an instance rule scoped to a different instance' do + rule_instance = create(:instance) + member_instance = create(:instance) + cu = create(:course_manager, course: create(:course)) + ActsAsTenant.with_tenant(member_instance) do + create(:instance_user, :instructor, user: cu.user, instance: member_instance) + end + create(:course_assessment_marketplace_allowlist_rule, + rule_type: :instance, instance: rule_instance) + + expect(described_class.new.rows.map(&:user)).not_to include(cu.user) + end + + it 'lists every rule matching a user, not just the highest-precedence one' do + user = create(:user, email: 'both@schools.gov.sg') + create(:course_manager, course: create(:course), user: user) + user_rule = create(:course_assessment_marketplace_allowlist_rule, + rule_type: :user, user: user) + domain_rule = create(:course_assessment_marketplace_allowlist_rule, :for_email_domain, + email_domain: 'schools.gov.sg') + + row = described_class.new.rows.find { |r| r.user == user } + expect(row.allowed_by_rules.map(&:id)).to contain_exactly(user_rule.id, domain_rule.id) + end + + it 'orders a row\'s rules by rule id' do + user = create(:user, email: 'ordered@schools.gov.sg') + create(:course_manager, course: create(:course), user: user) + first = create(:course_assessment_marketplace_allowlist_rule, :for_email_domain, + email_domain: 'schools.gov.sg') + second = create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: user) + + row = described_class.new.rows.find { |r| r.user == user } + expect(row.allowed_by_rules.map(&:id)).to eq([first.id, second.id]) + end + + it 'lists a blocked user whose matching rule was removed, so the block stays reachable' do + cu = create(:course_manager, course: create(:course)) + block = create(:course_assessment_marketplace_access_block, user: cu.user) + # No allow-list rule matches them at all — without the block they would not be listed. + + row = described_class.new.rows.find { |r| r.user == cu.user } + expect(row).not_to be_nil + expect(row.allowed_by_rules).to be_empty + expect(row.block_id).to eq(block.id) + expect(row.blocked?).to be(true) + end + + it 'lists a blocked user who is no longer baseline-eligible at all' do + user = create(:user) # manages nothing, staff nowhere + create(:course_assessment_marketplace_access_block, user: user) + + row = described_class.new.rows.find { |r| r.user == user } + expect(row).not_to be_nil + expect(row.course_count).to eq(0) + expect(row.instance_role).to be_nil + end + + describe '#allowed_user_ids' do + it 'returns baseline users cleared by a rule, and excludes uncleared ones' do + cleared = create(:course_manager, course: create(:course)) + uncleared = create(:course_manager, course: create(:course)) + create(:course_assessment_marketplace_allowlist_rule, + rule_type: :user, user: cleared.user) + + ids = described_class.new.allowed_user_ids + expect(ids).to include(cleared.user.id) + expect(ids).not_to include(uncleared.user.id) + end + + it 'still counts a blocked user as allowed — a block is not an allow-list decision' do + cu = create(:course_manager, course: create(:course)) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: cu.user) + create(:course_assessment_marketplace_access_block, user: cu.user) + + expect(described_class.new.allowed_user_ids).to include(cu.user.id) + end + + # Guards the `everyone?` branch: without it, collapsing the method to `rules_by_user.keys` + # would silently regress open-to-everyone into "only explicitly matched users". + it 'includes every baseline user when an everyone rule exists, not only rule-matched ones' do + first = create(:course_manager, course: create(:course)) + second = create(:course_manager, course: create(:course)) + create(:course_assessment_marketplace_allowlist_rule, :everyone) + + ids = described_class.new.allowed_user_ids + expect(ids).to include(first.user.id, second.user.id) + end + end + + describe 'system administrators' do + it 'lists an admin who manages nothing and matches no rule' do + admin = create(:administrator) + + row = described_class.new.rows.find { |r| r.user == admin } + expect(row).not_to be_nil + expect(row.system_admin?).to be(true) + expect(row.allowed_by_rules).to be_empty + end + + it 'keeps listing an admin as blocked when a block exists' do + # A block row cannot actually revoke a sysadmin's bypass, but an orphaned one must stay + # visible and clearable — same contract as any other blocked user. + admin = create(:administrator) + create(:course_assessment_marketplace_access_block, user: admin) + + row = described_class.new.rows.find { |r| r.user == admin } + expect(row.system_admin?).to be(true) + expect(row.blocked?).to be(true) + end + + it 'still records the rules that match an admin' do + admin = create(:administrator) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: admin) + + row = described_class.new.rows.find { |r| r.user == admin } + expect(row.system_admin?).to be(true) + expect(row.allowed_by_rules.map(&:rule_type)).to eq(['user']) + end + + it 'does not mark a non-admin as one' do + cu = create(:course_manager, course: create(:course)) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: cu.user) + + row = described_class.new.rows.find { |r| r.user == cu.user } + expect(row.system_admin?).to be(false) + end + end + + describe '#summary' do + it 'counts effective access and blocked separately, and reports the mode' do + active = create(:course_manager, course: create(:course)) + blocked = create(:course_manager, course: create(:course)) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: active.user) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: blocked.user) + create(:course_assessment_marketplace_access_block, user: blocked.user) + + # Admins are always listed and always count as having access, and the test DB carries at + # least the seeded one, so the expectation is relative to however many exist. + admins = User.administrator.count + summary = described_class.new.summary + expect(summary[:total_with_access]).to eq(1 + admins) + expect(summary[:total_blocked]).to eq(1) + expect(summary[:open_to_everyone]).to be(false) + end + + it 'reports open_to_everyone when an everyone rule exists' do + create(:course_assessment_marketplace_allowlist_rule, :everyone) + expect(described_class.new.summary[:open_to_everyone]).to be(true) + end + end + end +end diff --git a/spec/models/course/assessment/marketplace/allowlist_rule_spec.rb b/spec/models/course/assessment/marketplace/allowlist_rule_spec.rb new file mode 100644 index 00000000000..17b37957f7c --- /dev/null +++ b/spec/models/course/assessment/marketplace/allowlist_rule_spec.rb @@ -0,0 +1,260 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe Course::Assessment::Marketplace::AllowlistRule, type: :model do + let!(:instance) { Instance.default } + + before do + Course::Assessment::Marketplace::AllowlistRule.delete_all + User::Email.delete_all + end + + with_tenant(:instance) do + describe 'validations' do + it 'requires user for a user rule' do + rule = build(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: nil) + expect(rule).not_to be_valid + expect(rule.errors[:user]).to be_present + end + + it 'requires email_domain for an email_domain rule' do + rule = build(:course_assessment_marketplace_allowlist_rule, + rule_type: :email_domain, email_domain: nil) + expect(rule).not_to be_valid + expect(rule.errors[:email_domain]).to be_present + end + + it 'requires instance for an instance rule' do + rule = build(:course_assessment_marketplace_allowlist_rule, rule_type: :instance, instance: nil) + expect(rule).not_to be_valid + expect(rule.errors[:instance]).to be_present + end + + it 'is valid as an everyone rule with no target fields' do + rule = build(:course_assessment_marketplace_allowlist_rule, :everyone) + expect(rule).to be_valid + end + + it 'allows only one everyone rule' do + create(:course_assessment_marketplace_allowlist_rule, :everyone) + duplicate = build(:course_assessment_marketplace_allowlist_rule, :everyone) + expect(duplicate).not_to be_valid + expect(duplicate.errors[:rule_type]).to be_present + end + end + + describe '.grants_access?' do + it 'is false for a nil user' do + expect(described_class.grants_access?(nil)).to be(false) + end + + it 'is false when no rule matches' do + user = create(:user) + create(:course_assessment_marketplace_allowlist_rule, :for_email_domain, + email_domain: 'nomatch.example') + expect(described_class.grants_access?(user)).to be(false) + end + + it 'matches an explicit user rule' do + user = create(:user) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: user) + expect(described_class.grants_access?(user)).to be(true) + end + + it 'matches an instance rule when the user belongs to that instance' do + user = create(:user) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :instance, instance: instance) + expect(described_class.grants_access?(user)).to be(true) + end + + it 'does not match an instance rule for another instance under the current tenant' do + user = create(:user) + other_instance = create(:instance) + ActsAsTenant.with_tenant(other_instance) { InstanceUser.create!(user: user) } + create(:course_assessment_marketplace_allowlist_rule, rule_type: :instance, + instance: other_instance) + # `user.instance_users` is tenant-scoped (acts_as_tenant), so an instance rule grants + # access only while browsing the allow-listed instance — membership elsewhere is invisible. + expect(described_class.grants_access?(user)).to be(false) + end + + it 'matches an email-domain rule case-insensitively' do + user_email = create(:user_email, email: 'testuser@Schools.GOV.sg') + user = user_email.user + create(:course_assessment_marketplace_allowlist_rule, :for_email_domain, + email_domain: 'schools.gov.sg') + expect(described_class.grants_access?(user)).to be(true) + end + + it 'does not match a different email domain' do + user_email = create(:user_email, email: 'testuser@other.edu') + user = user_email.user + create(:course_assessment_marketplace_allowlist_rule, :for_email_domain, + email_domain: 'schools.gov.sg') + expect(described_class.grants_access?(user)).to be(false) + end + + it 'matches any user when an everyone rule exists' do + user = create(:user) + create(:course_assessment_marketplace_allowlist_rule, :everyone) + expect(described_class.grants_access?(user)).to be(true) + end + + it 'is false for a nil user even when an everyone rule exists' do + create(:course_assessment_marketplace_allowlist_rule, :everyone) + expect(described_class.grants_access?(nil)).to be(false) + end + + it 'keeps granting a user rule after the user replaces their email (access is by user_id, not email)' do + user = create(:user) + original_email = user.email + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: user) + expect(described_class.grants_access?(user)).to be(true) + + # Retire the original email and attach a brand-new one — the same person, different address. + user.emails.where(email: original_email).delete_all + create(:user_email, user: user, email: 'moved@newdomain.example') + user.reload + + expect(described_class.grants_access?(user)).to be(true) + end + + it 'does not grant access via a different user\'s rule after this user replaces their email' do + user = create(:user) + other_user = create(:user) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: other_user) + + user.emails.where(email: user.email).delete_all + create(:user_email, user: user, email: 'moved@newdomain.example') + user.reload + + expect(described_class.grants_access?(user)).to be(false) + end + end + + describe 'email resolution for a user rule' do + it 'resolves a confirmed email to the owning user' do + target = create(:user, email: 'teacher@school.edu') + rule = build(:course_assessment_marketplace_allowlist_rule, rule_type: :user, + user: nil, email: 'teacher@school.edu') + expect(rule).to be_valid + expect(rule.user).to eq(target) + end + + it 'is case-insensitive on the entered email' do + target = create(:user, email: 'teacher@school.edu') + rule = build(:course_assessment_marketplace_allowlist_rule, rule_type: :user, + user: nil, email: ' Teacher@School.EDU ') + expect(rule).to be_valid + expect(rule.user).to eq(target) + end + + it 'is invalid with a clear message when no user has that email' do + rule = build(:course_assessment_marketplace_allowlist_rule, rule_type: :user, + user: nil, email: 'nobody@nowhere.test') + expect(rule).not_to be_valid + expect(rule.errors.full_messages).to include('No user with that email.') + end + + it 'does not also add a user-presence error when the email fails to resolve' do + rule = build(:course_assessment_marketplace_allowlist_rule, rule_type: :user, + user: nil, email: 'nobody@nowhere.test') + expect(rule).not_to be_valid + expect(rule.errors.full_messages).to eq(['No user with that email.']) + end + end + + describe 'duplicate rules' do + it 'rejects a second user rule for the same user' do + user = create(:user) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: user) + duplicate = build(:course_assessment_marketplace_allowlist_rule, + rule_type: :user, user: user) + + expect(duplicate).not_to be_valid + expect(duplicate.errors[:user_id]).to include('already has a rule.') + end + + it 'allows a user rule for a different user' do + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: create(:user)) + expect(build(:course_assessment_marketplace_allowlist_rule, + rule_type: :user, user: create(:user))).to be_valid + end + + it 'rejects a second instance rule for the same instance' do + other_instance = create(:instance) + create(:course_assessment_marketplace_allowlist_rule, + rule_type: :instance, instance: other_instance) + duplicate = build(:course_assessment_marketplace_allowlist_rule, + rule_type: :instance, instance: other_instance) + + expect(duplicate).not_to be_valid + expect(duplicate.errors[:instance_id]).to include('already has a rule.') + end + + it 'rejects a second email-domain rule for the same domain' do + create(:course_assessment_marketplace_allowlist_rule, + rule_type: :email_domain, email_domain: 'dupes.test') + duplicate = build(:course_assessment_marketplace_allowlist_rule, + rule_type: :email_domain, email_domain: 'dupes.test') + + expect(duplicate).not_to be_valid + expect(duplicate.errors[:email_domain]).to include('already has a rule.') + end + + it 'treats a differently-cased domain as the same rule' do + create(:course_assessment_marketplace_allowlist_rule, + rule_type: :email_domain, email_domain: 'dupes.test') + duplicate = build(:course_assessment_marketplace_allowlist_rule, + rule_type: :email_domain, email_domain: ' DUPES.TEST ') + + expect(duplicate).not_to be_valid + expect(duplicate.errors[:email_domain]).to include('already has a rule.') + end + + it 'normalizes the stored domain to stripped lowercase' do + rule = create(:course_assessment_marketplace_allowlist_rule, + rule_type: :email_domain, email_domain: ' MiXeD.TEST ') + expect(rule.reload.email_domain).to eq('mixed.test') + end + + # A user rule whose email resolves to nobody keeps user_id NULL, and Rails checks uniqueness + # as `user_id IS NULL` — which matches every instance and email-domain rule unless the check + # is scoped to rule_type. Unscoped, the admin gets a bogus "already has a rule." stacked on + # top of the real reason. (Verified by mutation: dropping `scope: :rule_type` fails this.) + it 'does not report a duplicate for an unresolvable email when other rule types exist' do + create(:course_assessment_marketplace_allowlist_rule, + rule_type: :instance, instance: create(:instance)) + rule = build(:course_assessment_marketplace_allowlist_rule, + rule_type: :user, user: nil, email: 'nobody@nowhere.test') + + expect(rule).not_to be_valid + expect(rule.errors[:user_id]).to be_empty + expect(rule.errors[:base]).to include('No user with that email.') + end + end + + describe 'when the targeted user is destroyed' do + # Same FK trap as the access-blocks table: a user rule pins the user row, so deleting an + # allow-listed user from the admin panel raised PG::ForeignKeyViolation. + it 'destroys the rule instead of raising a foreign-key violation' do + user = create(:user) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: user) + + expect { ActsAsTenant.without_tenant { user.destroy } }. + to change { described_class.count }.by(-1) + end + + it 'leaves rules that do not target that user alone' do + create(:course_assessment_marketplace_allowlist_rule, + rule_type: :email_domain, email_domain: 'keeps.test') + # Created under the tenant, destroyed without one (as the admin panel does): building a + # user inside `without_tenant` fails its own `instance_users` validation. + bystander = create(:user) + + expect { ActsAsTenant.without_tenant { bystander.destroy } }. + not_to(change { described_class.count }) + end + end + end +end diff --git a/spec/models/course/assessment/marketplace/rule_match_query_spec.rb b/spec/models/course/assessment/marketplace/rule_match_query_spec.rb new file mode 100644 index 00000000000..9fb78a59b0f --- /dev/null +++ b/spec/models/course/assessment/marketplace/rule_match_query_spec.rb @@ -0,0 +1,123 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe Course::Assessment::Marketplace::RuleMatchQuery, type: :model do + let!(:instance) { Instance.default } + + # Specs commit (use_transactional_fixtures is false repo-wide), so rows from previous runs persist + # and User::Email enforces uniqueness. Clear the fixed-domain addresses this file creates. + # + # The pattern must be LOWER()'d and must not anchor the '@': the uniqueness index is on + # `lower(email)` while SQL LIKE is case-sensitive, and one example deliberately uses a SUBDOMAIN + # (someone@sub.match-query.test). A '%@match-query.test' pattern misses both, leaving rows behind + # that collide on the next run. + before do + User::Email.where('LOWER(email) LIKE ?', '%match-query.test').delete_all + end + + with_tenant(:instance) do + describe 'a user rule' do + it 'matches only the targeted user, and only within the candidate set' do + target = create(:user) + other = create(:user) + rule = build(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: target) + + expect(described_class.new(rule).user_ids_within([target.id, other.id])). + to eq(Set[target.id]) + end + + it 'returns nothing when the targeted user is outside the candidate set' do + target = create(:user) + other = create(:user) + rule = build(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: target) + + expect(described_class.new(rule).user_ids_within([other.id])).to be_empty + end + end + + describe 'an instance rule' do + it 'matches candidates belonging to that instance, across tenants' do + member = create(:user) + outsider = create(:user) + other_instance = create(:instance) + ActsAsTenant.with_tenant(other_instance) do + create(:instance_user, :instructor, user: member, instance: other_instance) + end + rule = build(:course_assessment_marketplace_allowlist_rule, + rule_type: :instance, instance: other_instance) + + expect(described_class.new(rule).user_ids_within([member.id, outsider.id])). + to eq(Set[member.id]) + end + end + + describe 'an email-domain rule' do + it 'matches a candidate holding a confirmed email at that domain' do + user = create(:user, email: 'teacher@match-query.test') + rule = build(:course_assessment_marketplace_allowlist_rule, + rule_type: :email_domain, email_domain: 'match-query.test') + + expect(described_class.new(rule).user_ids_within([user.id])).to eq(Set[user.id]) + end + + it 'matches case-insensitively on both the rule and the address' do + user = create(:user, email: 'head@match-query.test') + # Force the stored address to mixed case directly. `create(:user, email: 'HEAD@...')` + # raises RecordNotUnique even on a fresh address: the write path inserts both the given + # and the normalized form, and the two collide under the `lower(email)` unique index. + # Legacy rows can still hold mixed case, and the query's LOWER(SPLIT_PART(...)) on the + # address side exists for exactly them — so this is the only way to reach that branch. + User::Email.where(user_id: user.id). + where('LOWER(email) = ?', 'head@match-query.test'). + update_all(email: 'HEAD@MATCH-QUERY.TEST') + rule = build(:course_assessment_marketplace_allowlist_rule, + rule_type: :email_domain, email_domain: 'Match-Query.TEST') + + expect(described_class.new(rule).user_ids_within([user.id])).to eq(Set[user.id]) + end + + it 'ignores an unconfirmed address at that domain' do + user = create(:user) # confirmed primary email at a non-matching domain + create(:user_email, :unconfirmed, email: 'pending@match-query.test', + user: user, primary: false) + rule = build(:course_assessment_marketplace_allowlist_rule, + rule_type: :email_domain, email_domain: 'match-query.test') + + expect(described_class.new(rule).user_ids_within([user.id])).to be_empty + end + + it 'does not match a different domain that merely shares a suffix' do + user = create(:user, email: 'someone@sub.match-query.test') + rule = build(:course_assessment_marketplace_allowlist_rule, + rule_type: :email_domain, email_domain: 'match-query.test') + + expect(described_class.new(rule).user_ids_within([user.id])).to be_empty + end + end + + describe 'an everyone rule' do + it 'matches the whole candidate set' do + a = create(:user) + b = create(:user) + rule = build(:course_assessment_marketplace_allowlist_rule, :everyone) + + expect(described_class.new(rule).user_ids_within([a.id, b.id])).to eq(Set[a.id, b.id]) + end + end + + it 'returns an empty set for an empty candidate list without querying' do + rule = build(:course_assessment_marketplace_allowlist_rule, :everyone) + expect(described_class.new(rule).user_ids_within([])).to be_empty + end + + it 'treats an unsaved rule identically to a persisted one' do + target = create(:user) + unsaved = build(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: target) + persisted = create(:course_assessment_marketplace_allowlist_rule, + rule_type: :user, user: target) + + expect(described_class.new(unsaved).user_ids_within([target.id])). + to eq(described_class.new(persisted).user_ids_within([target.id])) + end + end +end diff --git a/spec/models/course/assessment_marketplace_ability_spec.rb b/spec/models/course/assessment_marketplace_ability_spec.rb index 1dabaf126c8..4564124a913 100644 --- a/spec/models/course/assessment_marketplace_ability_spec.rb +++ b/spec/models/course/assessment_marketplace_ability_spec.rb @@ -19,6 +19,7 @@ context 'when the user is a course manager' do let(:course_user) { create(:course_manager, course: course) } let(:user) { course_user.user } + before { create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: user) } it { is_expected.to be_able_to(:access_marketplace, course) } it { is_expected.not_to be_able_to(:publish_to_marketplace, build(:assessment)) } @@ -37,5 +38,79 @@ let(:user) { course_user.user } it { is_expected.not_to be_able_to(:access_marketplace, course) } end + + context 'when the user is a course manager but is not allow-listed' do + let(:course_user) { create(:course_manager, course: course) } + let(:user) { course_user.user } + + # Load-bearing at the ability level: without the explicit `cannot`, the blanket + # `can :manage, Course` a manager holds would satisfy `:access_marketplace`. + it { is_expected.not_to be_able_to(:access_marketplace, course) } + end + + context 'when the user is an observer here but manages another course (person-level access)' do + let(:course_user) { create(:course_observer, course: course) } + let(:user) { course_user.user } + before do + create(:course_manager, course: create(:course), user: user) + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: user) + end + + it { is_expected.to be_able_to(:access_marketplace, course) } + it { is_expected.to be_able_to(:duplicate_from_marketplace, published_assessment) } + it { is_expected.to be_able_to(:preview_in_marketplace, published_assessment) } + end + + context 'when an allow-listed user manages no course at all' do + let(:course_user) { create(:course_observer, course: course) } + let(:user) { course_user.user } + before { create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: user) } + + it { is_expected.not_to be_able_to(:access_marketplace, course) } + end + + context 'when the user is an instance instructor who manages no course but is allow-listed' do + let!(:course_user) { create(:course_observer, course: course) } + let!(:user) { course_user.user } + before do + other_instance = create(:instance) + ActsAsTenant.with_tenant(other_instance) do + create(:instance_user, :instructor, user: user, instance: other_instance) + end + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: user) + end + + # Proves the second baseline branch: eligible via instance role, not via managing a course. + it { is_expected.to be_able_to(:access_marketplace, course) } + end + + context 'when the user is an instance instructor who manages no course and is not allow-listed' do + let!(:course_user) { create(:course_observer, course: course) } + let!(:user) { course_user.user } + before do + other_instance = create(:instance) + ActsAsTenant.with_tenant(other_instance) do + create(:instance_user, :instructor, user: user, instance: other_instance) + end + end + + it { is_expected.not_to be_able_to(:access_marketplace, course) } + end + + context 'when an eligible, allow-listed manager is individually blocked' do + let(:course_user) { create(:course_manager, course: course) } + let(:user) { course_user.user } + before do + create(:course_assessment_marketplace_allowlist_rule, rule_type: :user, user: user) + create(:course_assessment_marketplace_access_block, user: user) + end + + it { is_expected.not_to be_able_to(:access_marketplace, course) } + + it 'regains access once the block is removed' do + Course::Assessment::Marketplace::AccessBlock.where(user_id: user.id).delete_all + expect(Ability.new(user, course, course_user)).to be_able_to(:access_marketplace, course) + end + end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index b7109e39b9d..0e8b01d6684 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -44,6 +44,67 @@ end end + describe '#course_manager_or_owner?' do + let(:user) { create(:user) } + + it 'is true when the user manages a course' do + create(:course_manager, course: create(:course), user: user) + expect(user.course_manager_or_owner?).to be(true) + end + + it 'is true when the user owns a course' do + create(:course_owner, course: create(:course), user: user) + expect(user.course_manager_or_owner?).to be(true) + end + + it 'is true when the user manages a course in a different instance' do + other_instance = create(:instance) + ActsAsTenant.with_tenant(other_instance) do + create(:course_manager, course: create(:course), user: user) + end + expect(user.course_manager_or_owner?).to be(true) + end + + it 'is false when the user only has non-manager course roles' do + create(:course_student, course: create(:course), user: user) + create(:course_observer, course: create(:course), user: user) + expect(user.course_manager_or_owner?).to be(false) + end + + it 'is false when the user is in no course' do + expect(user.course_manager_or_owner?).to be(false) + end + end + + describe '#instance_instructor_or_administrator?' do + # Eager: a lazy `let` would first run `create(:user)` inside the `with_tenant(other_instance)` + # block below, and `after_create :create_instance_user` would then give the user a normal + # InstanceUser in *that* instance — colliding with the instructor/administrator one we create. + let!(:user) { create(:user) } + + it 'is true when the user is an instructor in some instance' do + other_instance = create(:instance) + ActsAsTenant.with_tenant(other_instance) do + create(:instance_user, :instructor, user: user, instance: other_instance) + end + expect(user.instance_instructor_or_administrator?).to be(true) + end + + it 'is true when the user is an administrator in some instance' do + another_instance = create(:instance) + ActsAsTenant.with_tenant(another_instance) do + create(:instance_administrator, user: user, instance: another_instance) + end + expect(user.instance_instructor_or_administrator?).to be(true) + end + + it 'is false when the user is only a normal instance member' do + # `create(:user)` already gives a normal InstanceUser in the default instance via the + # after_create callback; there is no instructor/administrator membership anywhere. + expect(user.instance_instructor_or_administrator?).to be(false) + end + end + describe '#emails' do let(:user) { create(:user, emails_count: 5) } it 'unsets other email as primary when a new email is assigned' do