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

validate client id on refresh token "renewal" 

fixes CNVS-28858

also get rid of deprecated messages, and return a proper 401 for
invalid client credentials, as per spec

test plan:
 * have a developer key, go through the oauth flow, and get a refresh
   token
 * user the refresh token to get a new access token making sure it works
 * create a second developer key; try to get a new access token with
   your existing refresh token, but authenticating with the new
   client credentials
 * it should fail

Change-Id: Ide95e317dd8768abd9fb3ab4eb67225a6e58bbcb
Reviewed-on: https://gerrit.instructure.com/77634
Tested-by: Jenkins
Reviewed-by: Simon Williams <simon@instructure.com>
QA-Review: August Thornton <august@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
This commit is contained in:
Cody Cutrer 2016-04-21 14:37:24 -06:00
parent ac2578c360
commit 0db7878007
6 changed files with 67 additions and 57 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
app/controllers/oauth2_provider_controller.rb
View File

@ -82,12 +82,12 @@ class Oauth2ProviderController < ApplicationController
raise Canvas::Oauth::RequestError, :invalid_client_id unless provider.has_valid_key?
raise Canvas::Oauth::RequestError, :invalid_client_secret unless provider.is_authorized_by?(secret)
if grant_type == "authorization_code"
raise OAuth2RequestError :authorization_code_not_supplied unless params[:code]
token = provider.token_for(params[:code])
raise Canvas::Oauth::RequestError, :invalid_authorization_code unless token.is_for_valid_code?
raise Canvas::Oauth::RequestError, :incorrect_client unless token.key.id == token.client_id
token.create_access_token_if_needed(value_to_boolean(params[:replace_tokens]))
Canvas::Oauth::Token.expire_code(params[:code])
@ -97,6 +97,7 @@ class Oauth2ProviderController < ApplicationController
token = provider.token_for_refresh_token(params[:refresh_token])
# token = AccessToken.authenticate_refresh_token(params[:refresh_token])
raise Canvas::Oauth::RequestError, :invalid_refresh_token unless token
raise Canvas::Oauth::RequestError, :incorrect_client unless token.access_token.developer_key_id == token.key.id
token.access_token.regenerate_access_token
else
raise Canvas::Oauth::RequestError, :unsupported_grant_type
@ -114,6 +115,7 @@ class Oauth2ProviderController < ApplicationController
private
def oauth_error(exception)
response['WWW-Authenticate'] = 'Canvas OAuth 2.0' if exception.http_status == 401
return render(exception.to_render_data)
end

66
lib/canvas/oauth/request_error.rb
View File

@ -4,89 +4,81 @@ module Canvas::Oauth
invalid_client_id: {
error: :invalid_client,
error_description: "unknown client",
http_status: 400,
legacy: "invalid client_id"
},
http_status: 401
}.freeze,
invalid_client_secret: {
error: :invalid_client,
error_description: "invalid client",
http_status: 400,
legacy: "invalid client_secret"
},
http_status: 401
}.freeze,
invalid_redirect: {
error: :invalid_request,
error_description: "redirect_uri does not match client settings",
http_status: 400,
legacy: "invalid redirect_uri"
},
error_description: "redirect_uri does not match client settings"
}.freeze,
invalid_refresh_token: {
error: :invalid_request,
error_description: "refresh_token not found",
http_status: 400,
},
error_description: "refresh_token not found"
}.freeze,
invalid_authorization_code: {
error: :invalid_request,
error_description: "client does not have access to specified account",
http_status: 400,
legacy: "invalid code"
},
error: :invalid_grant,
error_description: "authorization_code not found"
}.freeze,
incorrect_client: {
error: :invalid_grant,
error_description: "incorrect client"
}.freeze,
authorization_code_not_supplied: {
error: :invalid_request,
error_description: "You must provide the code parameter when using the authorization_code grant type",
http_status: 400
},
error_description: "You must provide the code parameter when using the authorization_code grant type"
}.freeze,
refresh_token_not_supplied: {
error: :invalid_request,
error_description: "You must provide the refresh_token parameter when using the refresh_token grant type",
http_status: 400
},
error_description: "You must provide the refresh_token parameter when using the refresh_token grant type"
}.freeze,
unsupported_grant_type: {
error: :unsupported_grant_type,
error_description: "The grant_type you requested is not currently supported",
http_status: 400
},
error_description: "The grant_type you requested is not currently supported"
}.freeze,
client_not_authorized_for_account: {
error: :invalid_scope,
error_description: "client does not have access to specified account",
http_status: 401
}
}
}.freeze
}.freeze
def initialize(message)
@message = message
end
def to_json
# since we send back there error in the message key instead of the standard error
# at some point we should stop sending the message key
json_response = {
def as_json
{
error: error_map[:error],
error_description: error_map[:error_description]
}
json_response[:message] = error_map[:legacy] if error_map[:legacy]
json_response
end
def to_render_data
{
status: http_status,
json: to_json
json: as_json
}
end
private
def http_status
error_map[:http_status] || 400
end
private
def error_map
ERROR_MAP[@message]
end

2
lib/canvas/oauth/token.rb
View File

@ -20,7 +20,7 @@ module Canvas::Oauth
end
def is_for_valid_code?
code_data.present? && client_id == key.id
code_data.present?
end
def client_id

4
spec/apis/auth_spec.rb
View File

@ -353,14 +353,14 @@ describe "API Authentication", type: :request do
it "should require the developer key to have a redirect_uri" do
get "/login/oauth2/auth", :response_type => 'code', :client_id => @client_id, :redirect_uri => "http://www.example.com/oauth2response"
expect(response).to be_client_error
expect(response.body).to match /invalid redirect_uri/
expect(response.body).to match /redirect_uri/
end
it "should require the redirect_uri domains to match" do
@key.update_attribute :redirect_uri, 'http://www.example2.com/oauth2response'
get "/login/oauth2/auth", :response_type => 'code', :client_id => @client_id, :redirect_uri => "http://www.example.com/oauth2response"
expect(response).to be_client_error
expect(response.body).to match /invalid redirect_uri/
expect(response.body).to match /redirect_uri/
@key.update_attribute :redirect_uri, 'http://www.example.com/oauth2response'
get "/login/oauth2/auth", :response_type => 'code', :client_id => @client_id, :redirect_uri => "http://www.example.com/oauth2response"

43
spec/controllers/oauth2_provider_controller_spec.rb
View File

@ -22,16 +22,17 @@ describe Oauth2ProviderController do
describe 'GET auth' do
let_once(:key) { DeveloperKey.create! :redirect_uri => 'https://example.com' }
it 'renders a 400 when there is no client_id' do
it 'renders a 401 when there is no client_id' do
get :auth
assert_status(400)
expect(response.body).to match /invalid client_id/
assert_status(401)
expect(response.body).to match /unknown client/
expect(response['WWW-Authenticate']).to_not be_blank
end
it 'renders 400 on a bad redirect_uri' do
get :auth, :client_id => key.id
assert_status(400)
expect(response.body).to match /invalid redirect_uri/
expect(response.body).to match /redirect_uri does not match/
end
it 'redirects to the login url' do
@ -116,23 +117,23 @@ describe Oauth2ProviderController do
redis
end
it 'renders a 400 if theres no client_id' do
it 'renders a 401 if theres no client_id' do
post :token
assert_status(400)
expect(response.body).to match /invalid client_id/
assert_status(401)
expect(response.body).to match /unknown client/
end
it 'renders a 400 if the secret is invalid' do
it 'renders a 401 if the secret is invalid' do
post :token, :client_id => key.id, :client_secret => key.api_key + "123"
assert_status(400)
expect(response.body).to match /invalid client_secret/
assert_status(401)
expect(response.body).to match /invalid client/
end
it 'renders a 400 if the provided code does not match a token' do
Canvas.stubs(:redis => redis)
post :token, :client_id => key.id, :client_secret => key.api_key, :code => "NotALegitCode"
assert_status(400)
expect(response.body).to match /invalid code/
expect(response.body).to match /authorization_code not found/
end
it 'outputs the token json if everything checks out' do
@ -143,6 +144,14 @@ describe Oauth2ProviderController do
expect(JSON.parse(response.body).keys.sort).to match_array(['access_token', 'refresh_token', 'user', 'expires_in'])
end
it 'renders a 400 if the provided code is for the wrong key' do
Canvas.stubs(:redis => redis)
key2 = DeveloperKey.create!
post :token, client_id: key2.id.to_s, client_secret: key2.api_key, grant_type: 'authorization_code', code: valid_code
assert_status(400)
expect(response.body).to match(/incorrect client/)
end
it 'default grant_type to authorization_code if none is supplied and code is present' do
redis.expects(:del).with(valid_code_redis_key).at_least_once
Canvas.stubs(:redis => redis)
@ -197,17 +206,29 @@ describe Oauth2ProviderController do
expect(json['access_token']).to_not eq access_token
end
it 'errors with a mismatched client id' do
old_token = user.access_tokens.create! :developer_key => key
refresh_token = old_token.plaintext_refresh_token
key2 = DeveloperKey.create!
post :token, client_id: key2.id, client_secret: key2.api_key, grant_type: "refresh_token", refresh_token: refresh_token
assert_status(400)
expect(response.body).to match(/incorrect client/)
end
it 'should be able to regenerate access_token multiple times' do
old_token = user.access_tokens.create! :developer_key => key
refresh_token = old_token.plaintext_refresh_token
access_token = old_token.full_token
post :token, client_id: key.id, client_secret: key.api_key, grant_type: "refresh_token", refresh_token: refresh_token
expect(response).to be_success
json = JSON.parse(response.body)
expect(json['access_token']).to_not eq access_token
access_token = json['access_token']
post :token, client_id: key.id, client_secret: key.api_key, grant_type: "refresh_token", refresh_token: refresh_token
expect(response).to be_success
json = JSON.parse(response.body)
expect(json['access_token']).to_not eq access_token
end

5
spec/lib/canvas/oauth/token_spec.rb
View File

@ -36,11 +36,6 @@ module Canvas::Oauth
expect(token.is_for_valid_code?).to be_falsey
end
it 'is false when the client id does not match the key id' do
stub_out_cache (key.id + 1)
expect(token.is_for_valid_code?).to be_falsey
end
it 'is true otherwise' do
expect(token.is_for_valid_code?).to be_truthy
end