Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Heartbeat Monitoring for Exams: Safe Exam Browser (SEB) browser authorisation #7542

Merged
merged 3 commits into from
Sep 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions app/channels/course/monitoring/heartbeat_channel.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@
session: @session,
user_agent: user_agent,
ip_address: ip_address,
generated_at: time_from(timestamp)
generated_at: time_from(timestamp),
seb_payload: data['sebPayload']
)

return unless heartbeat.save
Expand Down Expand Up @@ -128,7 +129,6 @@
end

def valid_heartbeat?(heartbeat)
@monitor.valid_secret?(heartbeat.user_agent) ||
Course::Assessment::MonitoringService.unblocked?(assessment_id, session)
heartbeat.valid_heartbeat? || Course::Assessment::MonitoringService.unblocked?(assessment_id, session)

Check warning on line 132 in app/channels/course/monitoring/heartbeat_channel.rb

View check run for this annotation

Codecov / codecov/patch

app/channels/course/monitoring/heartbeat_channel.rb#L132

Added line #L132 was not covered by tests
end
end
9 changes: 5 additions & 4 deletions app/channels/course/monitoring/live_monitoring_channel.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ def view(data)
userAgent: heartbeat.user_agent,
ipAddress: heartbeat.ip_address,
generatedAt: heartbeat.generated_at,
isValid: @monitor.valid_secret?(heartbeat.user_agent)
isValid: heartbeat.valid_heartbeat?,
sebPayload: heartbeat.seb_payload
}.compact
end

Expand All @@ -61,7 +62,6 @@ def view(data)
def active_sessions_snapshots
@monitor.sessions.includes(:heartbeats, :creator).to_h do |session|
last_heartbeat = session.heartbeats.last
is_valid_secret = @monitor.valid_secret?(last_heartbeat&.user_agent)

course_user = course_users_hash[session.creator_id]
# This technically shouldn't happen, but can happen if someone is removed from
Expand All @@ -73,7 +73,7 @@ def active_sessions_snapshots
status: session.status,
misses: session.misses,
lastHeartbeatAt: last_heartbeat&.generated_at,
isValid: is_valid_secret,
isValid: last_heartbeat&.valid_heartbeat?,
userName: course_user.name,
submissionId: submission_ids_hash[session.creator_id],
stale: last_heartbeat&.stale
Expand Down Expand Up @@ -106,7 +106,8 @@ def broadcast_watch(users, snapshots, groups)
monitor: {
maxIntervalMs: @monitor.max_interval_ms,
offsetMs: @monitor.offset_ms,
hasSecret: @monitor.secret?
validates: @monitor.browser_authorization?,
browserAuthorizationMethod: @monitor.browser_authorization_method
}
}
end
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# frozen_string_literal: true
module Course::Assessment::Monitoring::SebPayloadConcern
extend ActiveSupport::Concern

private

def seb_payload_from_request(request)
url = request.headers['X-SafeExamBrowser-Url']
seb_config_key_hash = request.headers['X-SafeExamBrowser-ConfigKeyHash']
return unless url && seb_config_key_hash

# It's safe to not strip URL fragments (#) here because fragments are never sent to the server.
{ config_key_hash: seb_config_key_hash, url: url }

Check warning on line 13 in app/controllers/concerns/course/assessment/monitoring/seb_payload_concern.rb

View check run for this annotation

Codecov / codecov/patch

app/controllers/concerns/course/assessment/monitoring/seb_payload_concern.rb#L13

Added line #L13 was not covered by tests
end

def stub_heartbeat_from_request(request)
Course::Monitoring::Heartbeat.new(
user_agent: request.user_agent,
seb_payload: seb_payload_from_request(request)
)
end
end
17 changes: 14 additions & 3 deletions app/controllers/concerns/course/assessment/monitoring_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
module Course::Assessment::MonitoringConcern
extend ActiveSupport::Concern

include Course::Assessment::Monitoring::SebPayloadConcern

included do
alias_method :load_monitor, :monitor
alias_method :load_can_manage_monitor?, :can_manage_monitor?
Expand All @@ -11,14 +13,23 @@
before_action :load_can_manage_monitor?, only: [:index, :edit]
before_action :load_monitoring_component_enabled?, only: [:index, :edit]

before_action :raise_if_no_monitor, only: [:monitoring, :unblock_monitor]
before_action :raise_if_no_monitor, only: [:monitoring, :unblock_monitor, :seb_payload]
before_action :check_blocked_by_monitor, only: [:show]
end

def monitoring
authorize! :read, @monitor
end

# We need this endpoint because Safe Exam Browser (SEB) doesn't append keys in request headers
# of WebSocket connections.
def seb_payload
payload = seb_payload_from_request(request)
return head(:ok) unless payload

Check warning on line 28 in app/controllers/concerns/course/assessment/monitoring_concern.rb

View check run for this annotation

Codecov / codecov/patch

app/controllers/concerns/course/assessment/monitoring_concern.rb#L27-L28

Added lines #L27 - L28 were not covered by tests

render json: payload

Check warning on line 30 in app/controllers/concerns/course/assessment/monitoring_concern.rb

View check run for this annotation

Codecov / codecov/patch

app/controllers/concerns/course/assessment/monitoring_concern.rb#L30

Added line #L30 was not covered by tests
end

def unblock_monitor
session_password = unblock_monitor_params[:password]

Expand Down Expand Up @@ -55,7 +66,7 @@
end

def blocked_by_monitor?
cannot?(:read, monitor) && monitoring_service&.should_block?(request.user_agent) && !submitted_assessment?
cannot?(:read, monitor) && monitoring_service&.should_block?(request) && !submitted_assessment?
end

def monitoring_service
Expand All @@ -77,7 +88,7 @@
end

def should_disable_block?
[email protected]_password_protected? || monitor&.secret.blank?
[email protected]_password_protected? || !monitor&.browser_authorization?

Check warning on line 91 in app/controllers/concerns/course/assessment/monitoring_concern.rb

View check run for this annotation

Codecov / codecov/patch

app/controllers/concerns/course/assessment/monitoring_concern.rb#L91

Added line #L91 was not covered by tests
end

def submitted_assessment?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,6 @@ def check_blocked_by_monitor
end

def blocked_by_monitor?
should_monitor? && monitoring_service&.should_block?(request.user_agent)
should_monitor? && monitoring_service&.should_block?(request)
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,6 @@ def allow_students_create_read_update_sessions_heartbeats

can [:create, :read, :update], Course::Monitoring::Session, creator_id: user.id
can :create, Course::Monitoring::Heartbeat, session: { creator_id: user.id }
can :seb_payload, Course::Assessment, course_id: course.id
end
end
10 changes: 10 additions & 0 deletions app/models/course/monitoring/browser_authorization/base.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# frozen_string_literal: true
class Course::Monitoring::BrowserAuthorization::Base
def initialize(monitor)
@monitor = monitor
end

def valid?(monitor, heartbeat)
raise NotImplementedError

Check warning on line 8 in app/models/course/monitoring/browser_authorization/base.rb

View check run for this annotation

Codecov / codecov/patch

app/models/course/monitoring/browser_authorization/base.rb#L8

Added line #L8 was not covered by tests
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# frozen_string_literal: true
class Course::Monitoring::BrowserAuthorization::SebConfigKey < Course::Monitoring::BrowserAuthorization::Base
# @see https://safeexambrowser.org/developer/seb-config-key.html
def valid_heartbeat?(heartbeat)
seb_payload = heartbeat.seb_payload&.with_indifferent_access
return false unless seb_payload

url = seb_payload[:url]
hash = Digest::SHA256.hexdigest("#{url}#{@monitor.seb_config_key}")
hash == seb_payload[:config_key_hash]
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# frozen_string_literal: true
class Course::Monitoring::BrowserAuthorization::UserAgent < Course::Monitoring::BrowserAuthorization::Base
def valid_heartbeat?(heartbeat)
@monitor.secret? ? (heartbeat.user_agent&.include?(@monitor.secret) || false) : true
end
end
24 changes: 24 additions & 0 deletions app/models/course/monitoring/heartbeat.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,37 @@
validates :generated_at, presence: true
validates :stale, inclusion: { in: [true, false] }

validate :valid_seb_payload_if_exists

default_scope { order(:generated_at) }

before_save :update_session_misses

def valid_heartbeat?
session.monitor.valid_heartbeat?(self)

Check warning on line 18 in app/models/course/monitoring/heartbeat.rb

View check run for this annotation

Codecov / codecov/patch

app/models/course/monitoring/heartbeat.rb#L18

Added line #L18 was not covered by tests
end

private

SEB_PAYLOAD_SHAPE = { config_key_hash: String, url: String }.freeze

def update_session_misses
session.update_misses_after_heartbeat_saved!(self)
end

def filter_seb_payload(seb_payload)
seb_payload.slice(*SEB_PAYLOAD_SHAPE.keys)

Check warning on line 30 in app/models/course/monitoring/heartbeat.rb

View check run for this annotation

Codecov / codecov/patch

app/models/course/monitoring/heartbeat.rb#L30

Added line #L30 was not covered by tests
end

def valid_seb_payload?(seb_payload)
seb_payload.with_indifferent_access.tap do |payload|
return SEB_PAYLOAD_SHAPE.all? { |key, type| payload[key].instance_of?(type) }
end
end

def valid_seb_payload_if_exists
return if seb_payload.present? ? valid_seb_payload?(seb_payload) : true

errors.add(:seb_payload, :invalid_seb_payload)

Check warning on line 42 in app/models/course/monitoring/heartbeat.rb

View check run for this annotation

Codecov / codecov/patch

app/models/course/monitoring/heartbeat.rb#L42

Added line #L42 was not covered by tests
end
end
24 changes: 18 additions & 6 deletions app/models/course/monitoring/monitor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
class Course::Monitoring::Monitor < ApplicationRecord
DEFAULT_MIN_INTERVAL_MS = 3000

enum browser_authorization_method: { user_agent: 0, seb_config_key: 1 }

has_one :assessment, class_name: 'Course::Assessment', inverse_of: :monitor
has_many :sessions, class_name: 'Course::Monitoring::Session', inverse_of: :monitor

Expand All @@ -10,13 +12,17 @@
validates :max_interval_ms, numericality: { only_integer: true, greater_than: 0 }
validates :offset_ms, numericality: { only_integer: true, greater_than: 0 }
validates :blocks, inclusion: { in: [true, false] }
validates :browser_authorization, inclusion: { in: [true, false] }
validates :browser_authorization_method, presence: true

validate :max_interval_greater_than_min
validate :can_enable_only_when_password_protected
validate :can_block_only_when_has_secret_and_session_protected
validate :can_block_only_when_has_browser_authorization_and_session_protected
validate :seb_config_key_required_if_using_seb_config_key_browser_authorization

def valid_secret?(string)
secret? ? (string&.include?(secret) || false) : true
def valid_heartbeat?(heartbeat)
validator = "Course::Monitoring::BrowserAuthorization::#{browser_authorization_method.to_s.camelize}".constantize
validator.new(self).valid_heartbeat?(heartbeat)
end

# `Duplicator` already performed a shallow duplicate of the `other` monitor.
Expand All @@ -38,9 +44,15 @@
errors.add(:enabled, :must_be_password_protected)
end

def can_block_only_when_has_secret_and_session_protected
return unless blocks? && (secret.blank? || !assessment.session_password_protected?)
def can_block_only_when_has_browser_authorization_and_session_protected
return unless blocks? && (!browser_authorization? || !assessment.session_password_protected?)

errors.add(:blocks, :must_have_browser_authorization_and_session_protection)
end

def seb_config_key_required_if_using_seb_config_key_browser_authorization
return unless browser_authorization_method.to_sym == :seb_config_key && seb_config_key.blank?

errors.add(:blocks, :must_have_secret_and_session_protection)
errors.add(:seb_config_key, :required_if_using_seb_config_key_browser_authorization)

Check warning on line 56 in app/models/course/monitoring/monitor.rb

View check run for this annotation

Codecov / codecov/patch

app/models/course/monitoring/monitor.rb#L56

Added line #L56 was not covered by tests
end
end
18 changes: 15 additions & 3 deletions app/services/course/assessment/monitoring_service.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,20 @@
# frozen_string_literal: true
class Course::Assessment::MonitoringService
include Course::Assessment::Monitoring::SebPayloadConcern

class << self
def params
[:enabled, :secret, :min_interval_ms, :max_interval_ms, :offset_ms, :blocks]
[

Check warning on line 7 in app/services/course/assessment/monitoring_service.rb

View check run for this annotation

Codecov / codecov/patch

app/services/course/assessment/monitoring_service.rb#L7

Added line #L7 was not covered by tests
:enabled,
:min_interval_ms,
:max_interval_ms,
:offset_ms,
:blocks,
:browser_authorization,
:browser_authorization_method,
:secret,
:seb_config_key
]
end

def unblocked_browser_session_key(assessment_id)
Expand Down Expand Up @@ -35,8 +47,8 @@
end
end

def should_block?(string)
!unblocked? && monitor&.blocks? && !monitor&.valid_secret?(string)
def should_block?(request)
!unblocked? && monitor&.blocks? && !monitor&.valid_heartbeat?(stub_heartbeat_from_request(request))
end

def unblock(session_password)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# frozen_string_literal: true
class Course::Assessment::Submission::MonitoringService
include Course::Assessment::Monitoring::SebPayloadConcern

class << self
def for(submission, assessment, browser_session)
new(submission, assessment, browser_session) if assessment.monitor_id?
Expand Down Expand Up @@ -55,8 +57,8 @@ def listening?
@monitor.enabled? && session.listening?
end

def should_block?(string)
!unblocked? && @monitor&.blocks? && !@monitor&.valid_secret?(string)
def should_block?(request)
!unblocked? && @monitor&.blocks? && !@monitor&.valid_heartbeat?(stub_heartbeat_from_request(request))
end

private
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
# frozen_string_literal: true
json.monitoring do
json.enabled @monitor.enabled
json.secret @monitor.secret
json.min_interval_ms @monitor.min_interval_ms
json.max_interval_ms @monitor.max_interval_ms
json.offset_ms @monitor.offset_ms
json.blocks @monitor.blocks
json.browser_authorization @monitor.browser_authorization
json.browser_authorization_method @monitor.browser_authorization_method
json.secret @monitor.secret
json.seb_config_key @monitor.seb_config_key
end
39 changes: 38 additions & 1 deletion client/app/api/Base.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import axios, { AxiosInstance, AxiosResponse } from 'axios';
import axios, {
AxiosInstance,
AxiosResponse,
InternalAxiosRequestConfig,
} from 'axios';
import { getUserToken } from 'utilities/authentication';

import { syncSignals } from 'lib/hooks/unread';
Expand All @@ -21,6 +25,37 @@ const updateSignalsIfPresentIn = (response: AxiosResponse): void => {
syncSignals(JSON.parse(signals));
};

const getAbsoluteURLWithoutHashFromAxiosRequestConfig = (
config: InternalAxiosRequestConfig,
): string => {
const url = new URL(window.location.href);
url.pathname = config.url!;
url.hash = '';

Object.entries(config.params).forEach(([key, value]) =>
url.searchParams.set(key, value as string),
);

return url.toString();
};

/**
* We need this because Safe Exam Browser (SEB) only appends the config key hash in the
* request headers as `X-SafeExamBrowser-ConfigKeyHash` without the original URL used
* to hash it.
*
* The server shouldn't simply take the received request's URL because it's possible
* that the server sits behind a reverse proxy and only receives the request via a
* proxied internal URL. The safest way to ensure the server can correctly verify the
* config key hash is to also include the request URL at request time.
*/
const appendRequestURLIfOnSEB = (config: InternalAxiosRequestConfig): void => {
if (!navigator.userAgent.includes('SEB/')) return;

config.headers['X-SafeExamBrowser-Url'] =
getAbsoluteURLWithoutHashFromAxiosRequestConfig(config);
};

const getAuthorizationToken = (): string => {
const userToken = getUserToken();
return `Bearer ${userToken}`;
Expand Down Expand Up @@ -57,9 +92,11 @@ export default class BaseAPI {

client.interceptors.request.use(async (config) => {
config.withCredentials = true;
appendRequestURLIfOnSEB(config);
if (config.method === 'get') return config;

config.headers['X-CSRF-Token'] = await this.#getAndSaveCSRFToken();

return config;
});

Expand Down
Loading