diff --git a/app/controllers/openid_connect/authorizations_controller.rb b/app/controllers/openid_connect/authorizations_controller.rb index 54f118715..3b02cc625 100644 --- a/app/controllers/openid_connect/authorizations_controller.rb +++ b/app/controllers/openid_connect/authorizations_controller.rb @@ -1,8 +1,7 @@ class OpenidConnect::AuthorizationsController < ApplicationController rescue_from Rack::OAuth2::Server::Authorize::BadRequest do |e| - @error = e - print e.backtrace[0,10].join("\n") - render json: {error: :error, status: e.status} #error_message: e.message + logger.info e.backtrace[0,10].join("\n") + render json: {error: e.message || :error, status: e.status} end before_action :authenticate_user! @@ -19,21 +18,34 @@ private def request_authorization_consent_form endpoint = OpenidConnect::Authorization::EndpointStartPoint.new(current_user) - endpoint.call(request.env) - @client, @response_type, @redirect_uri, @scopes, @request_object = *[ - endpoint.client, endpoint.response_type, endpoint.redirect_uri, endpoint.scopes, endpoint.request_object - ] - saveRequestParameters - render :new + handleStartPointResponse(endpoint) + end + + def handleStartPointResponse(endpoint) + status, header, response = *endpoint.call(request.env) + if response.redirect? + redirect_to header['Location'] + else + @client, @response_type, @redirect_uri, @scopes, @request_object = *[ + endpoint.client, endpoint.response_type, endpoint.redirect_uri, endpoint.scopes, endpoint.request_object + ] + saveRequestParameters + render :new + end end def process_authorization_consent(approvedString) endpoint = OpenidConnect::Authorization::EndpointConfirmationPoint.new(current_user, to_boolean(approvedString)) restoreRequestParameters(endpoint) + handleConfirmationPointResponse(endpoint) + end + + def handleConfirmationPointResponse(endpoint) status, header, response = *endpoint.call(request.env) redirect_to header['Location'] end + def saveRequestParameters session[:client_id], session[:response_type], session[:redirect_uri], session[:scopes], session[:request_object] = @client.client_id, @response_type, @redirect_uri, @scopes.collect { |scope| scope.name }, @request_object diff --git a/config/routes.rb b/config/routes.rb index 520068e76..d1b1b1c7c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -236,8 +236,11 @@ Diaspora::Application.routes.draw do #OpenID Connect & OAuth namespace :openid_connect do resources :clients, only: :create - resources :authorizations, only: [:new, :create] post 'access_tokens', to: proc { |env| OpenidConnect::TokenEndpoint.new.call(env) } + + # Authorization Servers MUST support the use of the HTTP GET and POST methods at the Authorization Endpoint (see http://openid.net/specs/openid-connect-core-1_0.html#AuthResponseValidation). + resources :authorizations, only: [:new, :create] + post 'authorizations/new', to: 'authorizations#new' end api_version(module: "Api::V0", path: {value: "api/v0"}, default: true) do diff --git a/lib/openid_connect/authorization/endpoint_start_point.rb b/lib/openid_connect/authorization/endpoint_start_point.rb index 8fa6e4fa6..d8fae10c4 100644 --- a/lib/openid_connect/authorization/endpoint_start_point.rb +++ b/lib/openid_connect/authorization/endpoint_start_point.rb @@ -20,7 +20,7 @@ module OpenidConnect end def buildScopes(req) @scopes = req.scope.inject([]) do |_scopes_, scope| - _scopes_ << Scope.find_by_name(scope) or req.invalid_scope! "Unknown scope: #{scope}" + _scopes_ << (Scope.find_by_name(scope) or req.invalid_scope! "Unknown scope: #{scope}") end end end diff --git a/spec/controllers/openid_connect/authorizations_controller_spec.rb b/spec/controllers/openid_connect/authorizations_controller_spec.rb index 7852408ca..fedc62e60 100644 --- a/spec/controllers/openid_connect/authorizations_controller_spec.rb +++ b/spec/controllers/openid_connect/authorizations_controller_spec.rb @@ -1,7 +1,8 @@ require 'spec_helper' describe OpenidConnect::AuthorizationsController, type: :controller do - let!(:client) { OAuthApplication.create!(redirect_uris: ["http://localhost:3000/"]) } + let!(:client) { OAuthApplication.create!(name: "Diaspora Test Client", redirect_uris: ["http://localhost:3000/"]) } + let!(:client_with_multiple_redirects) { OAuthApplication.create!(name: "Diaspora Test Client", redirect_uris: ["http://localhost:3000/","http://localhost/"]) } before do sign_in :user, alice @@ -10,23 +11,120 @@ describe OpenidConnect::AuthorizationsController, type: :controller do end describe "#new" do - render_views context "when valid parameters are passed" do - it "should return a form page" do - get :new, + render_views + context "as GET request" do + it "should return a form page" do + get :new, + { + client_id: client.client_id, + redirect_uri: "http://localhost:3000/", + response_type: "id_token", + scope: "openid", + nonce: SecureRandom.hex(16), + state: SecureRandom.hex(16) + } + expect(response.body).to match("Diaspora Test Client") + end + end + context "as POST request" do + it "should return a form page" do + post :new, + { + client_id: client.client_id, + redirect_uri: "http://localhost:3000/", + response_type: "id_token", + scope: "openid", + nonce: SecureRandom.hex(16), + state: SecureRandom.hex(16) + } + expect(response.body).to match("Diaspora Test Client") + end + end + end + context "when client id is missing" do + it "should return an bad request error" do + post :new, { - client_id: client.client_id, redirect_uri: "http://localhost:3000/", response_type: "id_token", scope: "openid", nonce: SecureRandom.hex(16), state: SecureRandom.hex(16) } - expect(response.body).to match("Approve") - expect(response.body).to match("Deny") + expect(response.body).to match("bad_request") + end + end + context "when redirect uri is missing" do + context "when only one redirect URL is pre-registered" do + it "should return a form pager" do + # Note this intentionally behavior diverts from OIDC spec http://openid.net/specs/openid-connect-core-1_0.html#AuthRequest + # See https://github.com/nov/rack-oauth2/blob/master/lib/rack/oauth2/server/authorize.rb#L63 + post :new, + { + client_id: client.client_id, + response_type: "id_token", + scope: "openid", + nonce: SecureRandom.hex(16), + state: SecureRandom.hex(16) + } + expect(response.body).to match("Diaspora Test Client") + end + end + end + context "when multiple redirect URLs are pre-registered" do + it "should return an invalid request error" do + post :new, + { + client_id: client_with_multiple_redirects.client_id, + response_type: "id_token", + scope: "openid", + nonce: SecureRandom.hex(16), + state: SecureRandom.hex(16) + } + expect(response.body).to match("bad_request") + end + end + context "when redirect URI does not match pre-registered URIs" do + it "should return an invalid request error" do + post :new, + { + client_id: client.client_id, + redirect_uri: "http://localhost:2000/", + response_type: "id_token", + scope: "openid", + nonce: SecureRandom.hex(16) + } + expect(response.body).to match("bad_request") + end + end + context "when an unsupported scope is passed in" do + it "should return an invalid scope error" do + post :new, + { + client_id: client.client_id, + redirect_uri: "http://localhost:3000/", + response_type: "id_token", + scope: "random", + nonce: SecureRandom.hex(16), + state: SecureRandom.hex(16) + } + expect(response.body).to match("error=invalid_scope") + end + end + context "when nonce is missing" do + it "should return an invalid request error" do + post :new, + { + client_id: client.client_id, + redirect_uri: "http://localhost:3000/", + response_type: "id_token", + scope: "openid", + state: SecureRandom.hex(16) + } + expect(response.location).to match("error=invalid_request") end end - # TODO: Implement tests for missing/invalid parameters end describe "#create" do @@ -38,16 +136,21 @@ describe OpenidConnect::AuthorizationsController, type: :controller do response_type: "id_token", scope: "openid", nonce: SecureRandom.hex(16), - state: SecureRandom.hex(16) + state: 4180930983 } end context "when authorization is approved" do - it "should return the id token in a fragment" do + before do post :create, { approve: "true" } - expect(response.location).to have_content("#id_token=") + end + it "should return the id token in a fragment" do + expect(response.location).to have_content("id_token=") + end + it "should return the passed in state" do + expect(response.location).to have_content("state=4180930983") end end context "when authorization is denied" do @@ -58,10 +161,10 @@ describe OpenidConnect::AuthorizationsController, type: :controller do } end it "should return an error in the fragment" do - expect(response.location).to have_content("#error=") + expect(response.location).to have_content("error=") end it "should NOT contain a id token in the fragment" do - expect(response.location).to_not have_content("#id_token=") + expect(response.location).to_not have_content("id_token=") end end end