MSFT Sync: expect "tenant not authorized" errors
This way if users have not granted permissions for our app, or have revoked permissions, the erros will not clog up Sentry or ErrorReports. (We can still have an alert for a low percentage of 200s from the API to catch the case where our creds are completely bad.) refs INTEROP-6716 flag=microsoft_group_enrollments_syncing Test plan: - Given a MicrosoftSync::Group g, run: ra = g.root_account old_tenant = ra.settings[:microsoft_sync_tenant] ra.settings[:microsoft_sync_tenant] = 'microsoft.onmicrosoft.com' ra.save! g.syncer_job.run_synchronously - Check that the job stopes with graceful cancel error, ApplicationNotAuthorizedForTenant. last_error should be set in the group and workflow_state should be "errored" but should not have been a stack trace on the console. - Set the tenant back and run another sync just to make sure normal syncs are still working: ra.settings[:microsoft_sync_tenant] = old_tenant ra.save! g.syncer_job.run_synchronously - The sync should run successully Change-Id: I9c9323a53672f6247ca62db45cd158bb2751ca0b Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/263903 Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com> Reviewed-by: Xander Moffatt <xmoffatt@instructure.com> QA-Review: Xander Moffatt <xmoffatt@instructure.com> Product-Review: Evan Battaglia <ebattaglia@instructure.com>
This commit is contained in:
parent
46fa9fa573
commit
806107933c
|
@ -34,6 +34,10 @@ module MicrosoftSync
|
|||
GROUP_USERS_ADD_BATCH_SIZE = 20
|
||||
STATSD_PREFIX = 'microsoft_sync.graph_service'
|
||||
|
||||
class ApplicationNotAuthorizedForTenant < StandardError
|
||||
include Errors::GracefulCancelErrorMixin
|
||||
end
|
||||
|
||||
attr_reader :tenant
|
||||
|
||||
def initialize(tenant)
|
||||
|
@ -60,7 +64,8 @@ module MicrosoftSync
|
|||
def add_users_to_group(group_id, members: [], owners: [])
|
||||
raise ArgumentError, 'Missing users to add to group' if members.empty? && owners.empty?
|
||||
if (n_total_additions = members.length + owners.length) > GROUP_USERS_ADD_BATCH_SIZE
|
||||
raise ArgumentError, "Only 20 users can be added at once. Got #{n_total_additions}."
|
||||
raise ArgumentError, "Only #{GROUP_USERS_ADD_BATCH_SIZE} users can be added at " \
|
||||
"once. Got #{n_total_additions}."
|
||||
end
|
||||
|
||||
body = {}
|
||||
|
@ -158,7 +163,9 @@ module MicrosoftSync
|
|||
end
|
||||
end
|
||||
|
||||
unless (200..299).cover?(response.code)
|
||||
if application_not_authorized_response?(response)
|
||||
raise ApplicationNotAuthorizedForTenant
|
||||
elsif !(200..299).cover?(response.code)
|
||||
raise MicrosoftSync::Errors::HTTPInvalidStatus.for(
|
||||
service: 'graph', tenant: tenant, response: response
|
||||
)
|
||||
|
@ -175,6 +182,16 @@ module MicrosoftSync
|
|||
|
||||
private
|
||||
|
||||
def application_not_authorized_response?(response)
|
||||
(
|
||||
response.code == 401 &&
|
||||
response.body.include?('The identity of the calling application could not be established.')
|
||||
) || (
|
||||
response.code == 403 &&
|
||||
response.body.include?('Required roles claim values are not provided')
|
||||
)
|
||||
end
|
||||
|
||||
def statsd_name(error=nil)
|
||||
name = case error
|
||||
when nil then 'success'
|
||||
|
|
|
@ -48,18 +48,20 @@ describe MicrosoftSync::GraphService do
|
|||
let(:url_path_prefix_for_statsd) { URI.parse(url).path.split('/')[2] }
|
||||
|
||||
shared_examples_for 'a graph service endpoint' do |opts={}|
|
||||
let(:statsd_tags) do
|
||||
{
|
||||
msft_endpoint: "#{http_method}_#{url_path_prefix_for_statsd}",
|
||||
status_code: response[:status].to_s
|
||||
}
|
||||
end
|
||||
|
||||
unless opts[:ignore_404]
|
||||
context 'with a 404 status code' do
|
||||
let(:response) { json_response(404, error: {message: 'uh-oh!'}) }
|
||||
|
||||
it 'raises an HTTPNotFound error' do
|
||||
expect(InstStatsd::Statsd).to receive(:increment).with(
|
||||
'microsoft_sync.graph_service.notfound',
|
||||
tags: {
|
||||
msft_endpoint: "#{http_method}_#{url_path_prefix_for_statsd}",
|
||||
status_code: '404'
|
||||
}
|
||||
)
|
||||
expect(InstStatsd::Statsd).to receive(:increment)
|
||||
.with('microsoft_sync.graph_service.notfound', tags: statsd_tags)
|
||||
expect { subject }.to raise_error(
|
||||
MicrosoftSync::Errors::HTTPNotFound,
|
||||
/Graph service returned 404 for tenant mytenant.*uh-oh!/
|
||||
|
@ -73,13 +75,8 @@ describe MicrosoftSync::GraphService do
|
|||
let(:response) { json_response(code, error: {message: 'uh-oh!'}) }
|
||||
|
||||
it 'raises an HTTPInvalidStatus with the code and message' do
|
||||
expect(InstStatsd::Statsd).to receive(:increment).with(
|
||||
'microsoft_sync.graph_service.error',
|
||||
tags: {
|
||||
msft_endpoint: "#{http_method}_#{url_path_prefix_for_statsd}",
|
||||
status_code: code.to_s
|
||||
}
|
||||
)
|
||||
expect(InstStatsd::Statsd).to receive(:increment)
|
||||
.with('microsoft_sync.graph_service.error', tags: statsd_tags)
|
||||
expect { subject }.to raise_error(
|
||||
MicrosoftSync::Errors::HTTPInvalidStatus,
|
||||
/Graph service returned #{code} for tenant mytenant.*uh-oh!/
|
||||
|
@ -92,13 +89,8 @@ describe MicrosoftSync::GraphService do
|
|||
let(:response) { json_response(429, error: {message: 'uh-oh!'}) }
|
||||
|
||||
it 'raises an HTTPTooManyRequests error and increments a "throttled" counter' do
|
||||
expect(InstStatsd::Statsd).to receive(:increment).with(
|
||||
'microsoft_sync.graph_service.throttled',
|
||||
tags: {
|
||||
msft_endpoint: "#{http_method}_#{url_path_prefix_for_statsd}",
|
||||
status_code: '429'
|
||||
}
|
||||
)
|
||||
expect(InstStatsd::Statsd).to receive(:increment)
|
||||
.with('microsoft_sync.graph_service.throttled', tags: statsd_tags)
|
||||
expect { subject }.to raise_error(
|
||||
MicrosoftSync::Errors::HTTPTooManyRequests,
|
||||
/Graph service returned 429 for tenant mytenant.*uh-oh!/
|
||||
|
@ -112,15 +104,48 @@ describe MicrosoftSync::GraphService do
|
|||
expect(HTTParty).to receive(http_method.to_sym).and_raise error
|
||||
expect(InstStatsd::Statsd).to receive(:increment).with(
|
||||
'microsoft_sync.graph_service.error',
|
||||
tags: {
|
||||
msft_endpoint: "#{http_method}_#{url_path_prefix_for_statsd}",
|
||||
status_code: 'unknown'
|
||||
}
|
||||
tags: statsd_tags.merge(status_code: 'unknown')
|
||||
)
|
||||
expect { subject }.to raise_error(error)
|
||||
end
|
||||
end
|
||||
|
||||
context 'with a 401 tenant unauthorized error' do
|
||||
let(:response) do
|
||||
json_response(401, error: {
|
||||
code: "Authorization_IdentityNotFound",
|
||||
message: "The identity of the calling application could not be established."
|
||||
})
|
||||
end
|
||||
|
||||
it 'raises an ApplicationNotAuthorizedForTenant error' do
|
||||
expect(InstStatsd::Statsd).to receive(:increment)
|
||||
.with('microsoft_sync.graph_service.error', tags: statsd_tags)
|
||||
expect { subject }.to raise_error do |e|
|
||||
expect(e).to be_a(described_class::ApplicationNotAuthorizedForTenant)
|
||||
expect(e).to be_a(MicrosoftSync::Errors::GracefulCancelErrorMixin)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'with a 403 tenant unauthorized error' do
|
||||
let(:response) do
|
||||
json_response(403, error: {
|
||||
code: "AccessDenied",
|
||||
message: "Required roles claim values are not provided."
|
||||
})
|
||||
end
|
||||
|
||||
it 'raises an ApplicationNotAuthorizedForTenant error' do
|
||||
expect(InstStatsd::Statsd).to receive(:increment)
|
||||
.with('microsoft_sync.graph_service.error', tags: statsd_tags)
|
||||
expect { subject }.to raise_error do |e|
|
||||
expect(e).to be_a(described_class::ApplicationNotAuthorizedForTenant)
|
||||
expect(e).to be_a(MicrosoftSync::Errors::GracefulCancelErrorMixin)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
it 'increments a success statsd metric on success' do
|
||||
expect(InstStatsd::Statsd).to receive(:increment).with(
|
||||
'microsoft_sync.graph_service.success',
|
||||
|
|
Loading…
Reference in New Issue