ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

refactor file domain verifier interface 

refs RECNVS-481

just rearrange the code to give an interface more amenable to extension.
minimal functional change in this commit. expected change in function:

* user_id in files domain redirect and files domain session is global
* files domain gives explicit 401 when verifier is invalid (tampered or
  expired) rather than ignoring it and proceeding as if absent

test-plan:
- in a canvas environment with a files domain configured
- have a course with an uploaded file
- be logged in as a user with permission to access that file
- attempt to preview the file in the course's files area; the access to
  the file should redirect through the files domain and should work
- copy the files domain url redirected to from the previous step, and
  change the user_id to a different user, then attempt to access it;
  should get an unauthorized response

Change-Id: Id0d04b80c856037b852139342f79b56f82888fa6
Reviewed-on: https://gerrit.instructure.com/151071
Tested-by: Jenkins
Reviewed-by: Jonathan Featherstone <jfeatherstone@instructure.com>
QA-Review: Collin Parrish <cparrish@instructure.com>
Product-Review: Jacob Fugal <jacob@instructure.com>
This commit is contained in:
Jacob Fugal 2018-05-22 13:32:58 -06:00
parent fef679230b
commit 84f1327928
9 changed files with 170 additions and 58 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
app/controllers/application_controller.rb
View File

@ -1701,13 +1701,11 @@ class ApplicationController < ActionController::Base
res = "#{request.protocol}#{host}"
shard.activate do
ts, sig = @current_user && @current_user.access_verifier
# add parameters so that the other domain can create a session that
# will authorize file access but not full app access. We need this in
# case there are relative URLs in the file that point to other pieces
# of content.
opts = { :user_id => @current_user.try(:id), :ts => ts, :sf_verifier => sig }
opts = generate_access_verifier
opts[:verifier] = verifier if verifier.present?
if download

24
app/controllers/files_controller.rb
View File

@ -192,16 +192,20 @@ class FilesController < ApplicationController
end
def check_file_access_flags
if params[:user_id] && params[:ts] && params[:sf_verifier]
user = api_find(User, params[:user_id]) if params[:user_id].present?
if user && user.valid_access_verifier?(params[:ts], params[:sf_verifier])
# attachment.rb checks for this session attribute when determining
# permissions, but it should be ignored by the rest of the models'
# permission checks
session['file_access_user_id'] = user.id
session['file_access_expiration'] = 1.hour.from_now.to_i
session[:permissions_key] = SecureRandom.uuid
end
begin
access_verifier = validate_access_verifier
rescue Users::AccessVerifier::InvalidVerifier
render_unauthorized_action
return false
end
if access_verifier[:user]
# attachment.rb checks for this session attribute when determining
# permissions, but it should be ignored by the rest of the models'
# permission checks
session['file_access_user_id'] = access_verifier[:user].global_id
session['file_access_expiration'] = 1.hour.from_now.to_i
session[:permissions_key] = SecureRandom.uuid
end
# These sessions won't get deleted when the user logs out since this
# is on a separate domain, so we've added our own (stricter) timeout.

8
app/helpers/application_helper.rb
View File

@ -915,6 +915,14 @@ module ApplicationHelper
end
def generate_access_verifier
Users::AccessVerifier.generate(user: @current_user)
end
def validate_access_verifier
Users::AccessVerifier.validate(params)
end
def file_access_user
if !@files_domain
@current_user

16
app/models/user.rb
View File

@ -1488,22 +1488,6 @@ class User < ActiveRecord::Base
true
end
def generate_access_verifier(ts)
require 'openssl'
digest = OpenSSL::Digest::MD5.new
OpenSSL::HMAC.hexdigest(digest, uuid, ts.to_s)
end
private :generate_access_verifier
def access_verifier
ts = Time.now.utc.to_i
[ts, generate_access_verifier(ts)]
end
def valid_access_verifier?(ts, sig)
ts.to_i > 5.minutes.ago.to_i && ts.to_i < 1.minute.from_now.to_i && sig == generate_access_verifier(ts.to_i)
end
def uuid
if !read_attribute(:uuid)
self.update_attribute(:uuid, CanvasSlug.generate_securish_uuid)

42
app/models/users/access_verifier.rb Normal file
View File

@ -0,0 +1,42 @@
#
# Copyright (C) 2018 - present Instructure, Inc.
#
# This file is part of Canvas.
#
# Canvas is free software: you can redistribute it and/or modify it under
# the terms of the GNU Affero General Public License as published by the Free
# Software Foundation, version 3 of the License.
#
# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
# details.
#
# You should have received a copy of the GNU Affero General Public License along
# with this program. If not, see <http://www.gnu.org/licenses/>.
require 'openssl'
module Users
module AccessVerifier
class InvalidVerifier < RuntimeError
end
def self.generate(claims)
return {} unless claims[:user]
ts = Time.now.utc.to_i.to_s
user = claims[:user]
signature = OpenSSL::HMAC.hexdigest(OpenSSL::Digest::MD5.new, user.uuid, ts)
{ ts: ts, user_id: user.global_id, sf_verifier: signature }
end
def self.validate(fields)
return {} if fields[:sf_verifier].blank?
ts = fields[:ts]&.to_i
raise InvalidVerifier unless ts > 5.minutes.ago.to_i && ts < 1.minute.from_now.to_i
user = User.where(id: fields[:user_id]).first
raise InvalidVerifier unless user && fields[:sf_verifier] == OpenSSL::HMAC.hexdigest(OpenSSL::Digest::MD5.new, user.uuid, ts.to_s)
return { user: user }
end
end
end

7
spec/controllers/application_controller_spec.rb
View File

@ -226,12 +226,7 @@ describe ApplicationController do
# safe_domain_file_url wants to use request.protocol
allow(controller).to receive(:request).and_return(double(:protocol => '', :host_with_port => ''))
@common_params = {
:user_id => nil,
:ts => nil,
:sf_verifier => nil,
:only_path => true
}
@common_params = { :only_path => true }
end
it "should include inline=1 in url by default" do

35
spec/controllers/files_controller_spec.rb
View File

@ -305,27 +305,27 @@ describe FilesController do
it "should remember most recent sf_verifier in session" do
user1 = user_factory(active_all: true)
file1 = user_file
ts1, sf_verifier1 = user1.access_verifier
verifier1 = Users::AccessVerifier.generate(user: user1)
user2 = user_factory(active_all: true)
file2 = user_file
ts2, sf_verifier2 = user2.access_verifier
verifier2 = Users::AccessVerifier.generate(user: user2)
# first verifier
user_session(user1)
get 'show', params: {:user_id => user1.id, :id => file1.id, :ts => ts1, :sf_verifier => sf_verifier1}
get 'show', params: verifier1.merge(id: file1.id)
expect(response).to be_success
expect(session[:file_access_user_id]).to eq user1.id
expect(session[:file_access_user_id]).to eq user1.global_id
expect(session[:file_access_expiration]).not_to be_nil
expect(session[:permissions_key]).not_to be_nil
permissions_key = session[:permissions_key]
# second verifier, should update session
get 'show', params: {:user_id => user2.id, :id => @file.id, :ts => ts2, :sf_verifier => sf_verifier2}
get 'show', params: verifier2.merge(id: file2.id)
expect(response).to be_success
expect(session[:file_access_user_id]).to eq user2.id
expect(session[:file_access_user_id]).to eq user2.global_id
expect(session[:file_access_expiration]).not_to be_nil
expect(session[:permissions_key]).not_to eq permissions_key
permissions_key = session[:permissions_key]
@ -333,12 +333,21 @@ describe FilesController do
# repeat access, even without verifier, should extend expiration (though
# we can't assert that, because milliseconds) and thus change
# permissions_key
get 'show', params: {:user_id => user2.id, :id => @file.id}
get 'show', params: {id: file2.id}
expect(response).to be_success
expect(session[:permissions_key]).not_to eq permissions_key
end
it "should reject invalid sf_verifiers" do
user = user_factory(active_all: true)
file = user_file
verifier = Users::AccessVerifier.generate(user: user)
get 'show', params: verifier.merge(id: file.id, sf_verifier: 'invalid')
expect(response).to be_redirect
expect(response.location).to match('login')
end
it "should set cache headers for non text files" do
get 'show', params: {:course_id => @course.id, :id => @file.id, :download => 1, :verifier => @file.uuid, :download_frd => 1}
expect(response.header["Cache-Control"]).to include "private"
@ -673,17 +682,17 @@ describe FilesController do
it "should skip verification for an account-context file" do
account_js_file
verifier = Attachments::Verification.new(@file).verifier_for_user(nil)
ts, sf_verifier = @teacher.access_verifier
get 'show_relative', params: { :download => 1, :inline => 1, :sf_verifier => sf_verifier, :ts => ts, :user_id => @teacher.id, :verifier => verifier, :account_id => @account.id, :file_id => @file.id, :file_path => @file.full_path }
file_verifier = Attachments::Verification.new(@file).verifier_for_user(nil)
user_verifier = Users::AccessVerifier.generate(user: @teacher)
get 'show_relative', params: user_verifier.merge(:download => 1, :inline => 1, :verifier => file_verifier, :account_id => @account.id, :file_id => @file.id, :file_path => @file.full_path)
expect(response).to be_success
end
it "should enforce verification for contexts other than account" do
course_file
verifier = Attachments::Verification.new(@file).verifier_for_user(nil)
ts, sf_verifier = @teacher.access_verifier
get 'show_relative', params: { :download => 1, :inline => 1, :sf_verifier => sf_verifier, :ts => ts, :user_id => @teacher.id, :verifier => verifier, :account_id => @account.id, :file_id => @file.id, :file_path => @file.full_path }
file_verifier = Attachments::Verification.new(@file).verifier_for_user(nil)
user_verifier = Users::AccessVerifier.generate(user: @teacher)
get 'show_relative', params: user_verifier.merge(:download => 1, :inline => 1, :verifier => file_verifier, :account_id => @account.id, :file_id => @file.id, :file_path => @file.full_path)
assert_unauthorized
end

24
spec/integration/files_spec.rb
View File

@ -40,10 +40,11 @@ describe FilesController do
get "http://test.host/files/#{@submission.attachment.id}/download", params: {:inline => '1', :verifier => @submission.attachment.uuid}
expect(response).to be_redirect
uri = URI.parse response['Location']
qs = Rack::Utils.parse_nested_query(uri.query)
qs = Rack::Utils.parse_nested_query(uri.query).with_indifferent_access
expect(uri.host).to eq 'files-test.host'
expect(uri.path).to eq "/files/#{@submission.attachment.id}/download"
expect(@me.valid_access_verifier?(qs['ts'], qs['sf_verifier'])).to be_truthy
expect{ Users::AccessVerifier.validate(qs) }.not_to raise_exception
expect(Users::AccessVerifier.validate(qs)[:user]).to eql(@me)
expect(qs['verifier']).to eq @submission.attachment.uuid
location = response['Location']
remove_user_session
@ -78,11 +79,12 @@ describe FilesController do
get "http://test.host/users/#{@me.id}/files/#{@att.id}/download"
expect(response).to be_redirect
uri = URI.parse response['Location']
qs = Rack::Utils.parse_nested_query(uri.query)
qs = Rack::Utils.parse_nested_query(uri.query).with_indifferent_access
expect(uri.host).to eq 'files-test.host'
# redirects to a relative url, since relative files are available in user context
expect(uri.path).to eq "/users/#{@me.id}/files/#{@att.id}/my%20files/unfiled/my-pic.png"
expect(@me.valid_access_verifier?(qs['ts'], qs['sf_verifier'])).to be_truthy
expect{ Users::AccessVerifier.validate(qs) }.not_to raise_exception
expect(Users::AccessVerifier.validate(qs)[:user]).to eql(@me)
location = response['Location']
remove_user_session
@ -156,10 +158,11 @@ describe FilesController do
get "http://test.host/courses/#{@course.id}/files/#{a1.id}/download", params: {:inline => '1'}
expect(response).to be_redirect
uri = URI.parse response['Location']
qs = Rack::Utils.parse_nested_query(uri.query)
qs = Rack::Utils.parse_nested_query(uri.query).with_indifferent_access
expect(uri.host).to eq 'files-test.host'
expect(uri.path).to eq "/courses/#{@course.id}/files/#{a1.id}/course%20files/test%20my%20file%3F%20hai!%26.png"
expect(@user.valid_access_verifier?(qs['ts'], qs['sf_verifier'])).to be_truthy
expect{ Users::AccessVerifier.validate(qs) }.not_to raise_exception
expect(Users::AccessVerifier.validate(qs)[:user]).to eql(@user)
expect(qs['verifier']).to be_nil
location = response['Location']
remove_user_session
@ -278,10 +281,11 @@ describe FilesController do
get url
expect(response).to be_redirect
uri = URI.parse response['Location']
qs = Rack::Utils.parse_nested_query(uri.query)
qs = Rack::Utils.parse_nested_query(uri.query).with_indifferent_access
expect(uri.host).to eq 'files-test.host'
expect(uri.path).to eq "/files/#{@att.id}/download"
expect(@user.valid_access_verifier?(qs['ts'], qs['sf_verifier'])).to be_truthy
expect{ Users::AccessVerifier.validate(qs) }.not_to raise_exception
expect(Users::AccessVerifier.validate(qs)[:user]).to eql(@user)
expect(qs['verifier']).to eq @att.uuid
location = response['Location']
remove_user_session
@ -381,8 +385,8 @@ describe FilesController do
user_factory(active_all: true)
user_session(@user)
ts, sf_verifier = @user.access_verifier
get "/files/#{att.id}", params: {:user_id => @user.id, :ts => ts, :sf_verifier => sf_verifier} # set the file access session tokens
user_verifier = Users::AccessVerifier.generate(user: @user)
get "/files/#{att.id}", params: user_verifier # set the file access session tokens
expect(session['file_access_user_id']).to be_present
get "/courses/#{@course.id}/files/#{att.id}/file_preview"

68
spec/models/users/access_verifier_spec.rb Normal file
View File

@ -0,0 +1,68 @@
#
# Copyright (C) 2018 - present Instructure, Inc.
#
# This file is part of Canvas.
#
# Canvas is free software: you can redistribute it and/or modify it under
# the terms of the GNU Affero General Public License as published by the Free
# Software Foundation, version 3 of the License.
#
# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
# details.
#
# You should have received a copy of the GNU Affero General Public License along
# with this program. If not, see <http://www.gnu.org/licenses/>.
require 'spec_helper'
require_dependency "users/access_verifier"
module Users
describe AccessVerifier do
describe "generate" do
let(:user) { double("user", uuid: "abcd", global_id: 1) }
it "returns an empty hash if no user claimed" do
expect(Users::AccessVerifier.generate(user: nil)).to eql({})
end
it "includes an sf_verifier field in the response" do
expect(Users::AccessVerifier.generate(user: user)).to have_key(:sf_verifier)
end
end
describe "validate" do
let(:user) { user_model }
it "returns empty set of verified claims if no sf_verifier present" do
expect(Users::AccessVerifier.validate({})).to eql({})
end
it "success on an issued verifier" do
verifier = Users::AccessVerifier.generate(user: user)
expect{ Users::AccessVerifier.validate(verifier) }.not_to raise_exception
end
it "returns verified user claim on success" do
verifier = Users::AccessVerifier.generate(user: user)
verified = Users::AccessVerifier.validate(verifier)
expect(verified).to have_key(:user)
expect(verified[:user]).to eql(user)
end
it "raises InvalidVerifier if too old" do
verifier = Users::AccessVerifier.generate(user: user)
Timecop.freeze(10.minutes.from_now) do
expect{ Users::AccessVerifier.validate(verifier) }.to raise_exception(Users::AccessVerifier::InvalidVerifier)
end
end
it "raises InvalidVerifier if tampered with user" do
verifier = Users::AccessVerifier.generate(user: user)
tampered = verifier.merge(sf_verifier: 'tampered')
expect{ Users::AccessVerifier.validate(tampered) }.to raise_exception(Users::AccessVerifier::InvalidVerifier)
end
end
end
end