diff --git a/app/controllers/api/v0/base_controller.rb b/app/controllers/api/v0/base_controller.rb index 0899a13a8..f95c9c5c7 100644 --- a/app/controllers/api/v0/base_controller.rb +++ b/app/controllers/api/v0/base_controller.rb @@ -1,5 +1,5 @@ class Api::V0::BaseController < ApplicationController - include OpenidConnect::Authentication + include OpenidConnect::ProtectedResourceEndpoint before_filter :require_access_token end diff --git a/app/controllers/api/v0/users_controller.rb b/app/controllers/api/v0/users_controller.rb index e57c7cd23..60bd14589 100644 --- a/app/controllers/api/v0/users_controller.rb +++ b/app/controllers/api/v0/users_controller.rb @@ -6,6 +6,6 @@ class Api::V0::UsersController < Api::V0::BaseController private def user - current_token.o_auth_application.user + current_token.user end end diff --git a/app/controllers/openid_connect/authorizations_controller.rb b/app/controllers/openid_connect/authorizations_controller.rb index fcbe04dfa..c10c137be 100644 --- a/app/controllers/openid_connect/authorizations_controller.rb +++ b/app/controllers/openid_connect/authorizations_controller.rb @@ -18,7 +18,7 @@ class AuthorizationsController < ApplicationController private def call_authorization_endpoint(is_create = false, approved = false) - endpoint = AuthorizationEndpoint.new current_user, is_create, approved + endpoint = AuthorizationEndpoint.new is_create, approved rack_response = *endpoint.call(request.env) @client, @response_type, @redirect_uri, @scopes, @_request_, @request_uri, @request_object = *[ endpoint.client, endpoint.response_type, endpoint.redirect_uri, endpoint.scopes, endpoint._request_, endpoint.request_uri, endpoint.request_object diff --git a/app/controllers/openid_connect/clients_controller.rb b/app/controllers/openid_connect/clients_controller.rb new file mode 100644 index 000000000..e067b9ab2 --- /dev/null +++ b/app/controllers/openid_connect/clients_controller.rb @@ -0,0 +1,30 @@ +class OpenidConnect::ClientsController < ApplicationController + + rescue_from OpenIDConnect::HttpError do |e| + rewriteHTTPErrorPageAsJSON(e) + end + rescue_from OpenIDConnect::ValidationFailed do |e| + rewriteValidationFailErrorPageAsJSON(e) + end + + def create + registrar = OpenIDConnect::Client::Registrar.new(request.url, params) + client = OAuthApplication.register! registrar + render json: client + end + +private + + def rewriteHTTPErrorPageAsJSON(e) + render json: { + error: :invalid_request, + error_description: e.message + }, status: 400 + end + def rewriteValidationFailErrorPageAsJSON(e) + render json: { + error: :invalid_client_metadata, + error_description: e.message + }, status: 400 + end +end diff --git a/app/models/o_auth_application.rb b/app/models/o_auth_application.rb index 0d85e25a9..f40816b32 100644 --- a/app/models/o_auth_application.rb +++ b/app/models/o_auth_application.rb @@ -4,4 +4,32 @@ class OAuthApplication < ActiveRecord::Base validates :client_id, presence: true, uniqueness: true validates :client_secret, presence: true + before_validation :setup, on: :create + def setup + self.client_id = SecureRandom.hex(16) + self.client_secret = SecureRandom.hex(32) + end + + class << self + def register!(registrarHash) + registrarHash.validate! + buildClientApplication(registrarHash) + end + + def buildClientApplication(registrarHash) + client = OAuthApplication.create! + client.attributes = filterNilValues(registrarHash) + client.save! + client + end + + def filterNilValues(registrarHash) + { + name: registrarHash.client_name, + redirect_uris: registrarHash.redirect_uris + }.delete_if do |key, value| + value.nil? + end + end + end end diff --git a/features/desktop/oauth_password_flow.feature b/features/desktop/oauth_password_flow.feature index 4e28d825f..2829ac296 100644 --- a/features/desktop/oauth_password_flow.feature +++ b/features/desktop/oauth_password_flow.feature @@ -1,19 +1,20 @@ -@javascript Feature: Access protected resources using password flow - # TODO: Add tests for expired access tokens Background: Given a user with username "kent" - Scenario: Valid bearer tokens sent via Authorization Request Header Field + Scenario: Invalid credentials to token endpoint + When I register a new client + And I send a post request from that client to the token endpoint using invalid credentials + Then I should receive an "invalid_grant" error - Scenario: Valid bearer tokens sent via Form Encoded Parameter - - Scenario: Valid bearer tokens sent via URI query parameter - When I send a post request to the token endpoint using "kent"'s credentials - And I use received valid bearer tokens to access user info via URI query parameter - Then I should receive "kent"'s id, username, and email - - Scenario: Invalid bearer tokens sent via URI query parameter - When I send a post request to the token endpoint using "kent"'s credentials - And I use invalid bearer tokens to access user info via URI query parameter + Scenario: Invalid bearer tokens sent + When I register a new client + And I send a post request from that client to the token endpoint using "kent"'s credentials + And I use invalid bearer tokens to access user info Then I should receive an "invalid_token" error + + Scenario: Valid password flow + When I register a new client + And I send a post request from that client to the token endpoint using "kent"'s credentials + And I use received valid bearer tokens to access user info + Then I should receive "kent"'s id, username, and email diff --git a/features/step_definitions/openid_steps.rb b/features/step_definitions/openid_steps.rb index 47fada788..a90cc35d3 100644 --- a/features/step_definitions/openid_steps.rb +++ b/features/step_definitions/openid_steps.rb @@ -1,37 +1,64 @@ -# Password has been hard coded as all test accounts seem to have a password of "password" -Given /^I send a post request to the token endpoint using "([^\"]*)"'s credentials$/ do |username| - user = User.find_by(username: username) - tokenEndpointURL = "/openid/access_tokens" - tokenEndpointURLQuery = "?grant_type=password&username=" + - user.username + - "&password=password&client_id=4&client_secret=azerty" - post tokenEndpointURL + tokenEndpointURLQuery +When /^I register a new client$/ do + clientRegistrationURL = "/openid_connect/clients" + post clientRegistrationURL, + { + redirect_uris: ["http://localhost:3000"] # Not actually used + } end -When /^I use received valid bearer tokens to access user info via URI query parameter$/ do +Given /^I send a post request from that client to the token endpoint using "([^\"]*)"'s credentials$/ do |username| + clientJSON = JSON.parse(last_response.body) + user = User.find_by(username: username) + tokenEndpointURL = "/openid_connect/access_tokens" + post tokenEndpointURL, + { + grant_type: "password", + username: user.username, + password: "password", # Password has been hard coded as all test accounts seem to have a password of "password" + client_id: clientJSON["o_auth_application"]["client_id"], + client_secret: clientJSON["o_auth_application"]["client_secret"] + } +end + +Given /^I send a post request from that client to the token endpoint using invalid credentials$/ do + clientJSON = JSON.parse(last_response.body) + tokenEndpointURL = "/openid_connect/access_tokens" + post tokenEndpointURL, + { + grant_type: "password", + username: User.find_by(username: "bob"), + password: "wrongpassword", + client_id: clientJSON["o_auth_application"]["client_id"], + client_secret: clientJSON["o_auth_application"]["client_secret"] + } +end + +When /^I use received valid bearer tokens to access user info$/ do accessTokenJson = JSON.parse(last_response.body) userInfoEndPointURL = "/api/v0/user/" - userInfoEndPointURLQuery = "?access_token=" + accessTokenJson["access_token"] - visit userInfoEndPointURL + userInfoEndPointURLQuery + get userInfoEndPointURL, + { + access_token: accessTokenJson["access_token"] + } end -When /^I use invalid bearer tokens to access user info via URI query parameter$/ do +When /^I use invalid bearer tokens to access user info$/ do userInfoEndPointURL = "/api/v0/user/" - userInfoEndPointURLQuery = "?access_token=" + SecureRandom.hex(32) - visit userInfoEndPointURL + userInfoEndPointURLQuery + get userInfoEndPointURL, + { + access_token: SecureRandom.hex(32) + } end Then /^I should receive "([^\"]*)"'s id, username, and email$/ do |username| + userInfoJson = JSON.parse(last_response.body) user = User.find_by_username(username) - expect(page).to have_content(user.username) - expect(page).to have_content(user.language) - expect(page).to have_content(user.email) + expect(userInfoJson["username"]).to have_content(user.username) + expect(userInfoJson["language"]).to have_content(user.language) + expect(userInfoJson["email"]).to have_content(user.email) end Then /^I should receive an "([^\"]*)" error$/ do |error_message| - expect(page).to have_content(error_message) -end - -Then /^I should see "([^\"]*)" in the content$/ do |content| - expect(page).to have_content(content) + userInfoJson = JSON.parse(last_response.body) + expect(userInfoJson["error"]).to have_content(error_message) end diff --git a/lib/openid_connect/authorization/endpoint.rb b/lib/openid_connect/authorization/endpoint.rb new file mode 100644 index 000000000..cf2b359f3 --- /dev/null +++ b/lib/openid_connect/authorization/endpoint.rb @@ -0,0 +1,50 @@ +module OpenidConnect + module Authorization + class Endpoint + attr_accessor :app, :user, :client, :redirect_uri, :response_type, :scopes, :_request_, :request_uri, :request_object + delegate :call, to: :app + + def initialize(current_user) + @user = current_user + @app = Rack::OAuth2::Server::Authorize.new do |req, res| + buildClient(req) + buildRedirectURI(req, res) + verifyNonce(req, res) + buildScopes(req) + buildRequestObject(req) + if OAuthApplication.available_response_types.include? Array(req.response_type).collect(&:to_s).join(' ') + handleResponseType(req, res) + else + req.unsupported_response_type! + end + end + end + def buildClient(req) + @client = OAuthApplication.find_by_client_id(req.client_id) || req.bad_request! + end + def buildRedirectURI(req, res) + res.redirect_uri = @redirect_uri = req.verify_redirect_uri!(@client.redirect_uris) + end + def verifyNonce(req, res) + if res.protocol_params_location == :fragment && req.nonce.blank? + req.invalid_request! 'nonce required' + end + end + def buildScopes(req) + @scopes = req.scope.inject([]) do |_scopes_, scope| + _scopes_ << Scope.find_by_name(scope) or req.invalid_scope! "Unknown scope: #{scope}" + end + end + def buildRequestObject(req) + @request_object = if (@_request_ = req.request).present? + OpenIDConnect::RequestObject.decode req.request, nil # @client.secret + elsif (@request_uri = req.request_uri).present? + OpenIDConnect::RequestObject.fetch req.request_uri, nil # @client.secret + end + end + def handleResponseType(req, res) + # Implemented by subclass + end + end + end +end diff --git a/lib/openid_connect/authorization_endpoint.rb b/lib/openid_connect/authorization_endpoint.rb deleted file mode 100644 index 61fbe11ce..000000000 --- a/lib/openid_connect/authorization_endpoint.rb +++ /dev/null @@ -1,12 +0,0 @@ -module OpenidConnect - class AuthorizationEndpoint - attr_accessor :app, :account, :client, :redirect_uri, :response_type, :scopes, :_request_, :request_uri, :request_object - delegate :call, to: :app - - def initialize(allow_approval = false, approved = false) - @app = Rack::OAuth2::Server::Authorize.new do |req, res| - req.unsupported_response_type! # TODO: not supported yet - end - end - end -end diff --git a/lib/openid_connect/authentication.rb b/lib/openid_connect/protected_resource_endpoint.rb similarity index 88% rename from lib/openid_connect/authentication.rb rename to lib/openid_connect/protected_resource_endpoint.rb index eca5573a0..fa840c18b 100644 --- a/lib/openid_connect/authentication.rb +++ b/lib/openid_connect/protected_resource_endpoint.rb @@ -1,8 +1,8 @@ module OpenidConnect - module Authentication + module ProtectedResourceEndpoint def self.included(klass) - klass.send :include, Authentication::Helper + klass.send :include, ProtectedResourceEndpoint::Helper end module Helper diff --git a/lib/openid_connect/token_endpoint.rb b/lib/openid_connect/token_endpoint.rb index 070ae4549..2f8e9c3d7 100644 --- a/lib/openid_connect/token_endpoint.rb +++ b/lib/openid_connect/token_endpoint.rb @@ -5,35 +5,43 @@ module OpenidConnect def initialize @app = Rack::OAuth2::Server::Token.new do |req, res| - case req.grant_type - when :password - user = User.find_for_database_authentication(username: req.username) - if user - o_auth_app = retrieveOrCreateNewClientApplication(req, user) - if o_auth_app && user.valid_password?(req.password) - res.access_token = o_auth_app.tokens.create!.bearer_token - else - req.invalid_grant! - end - else - req.invalid_grant! # TODO: Change to user login - end - else - res.unsupported_grant_type! + o_auth_app = retrieveClient(req) + if isAppValid(o_auth_app, req) + handleFlows(req, res) + else + req.invalid_client! end end end - def retrieveOrCreateNewClientApplication(req, user) - retrieveClient(req, user) || createClient(req, user) + def handleFlows(req, res) + case req.grant_type + when :password + handlePasswordFlow(req, res) + else + req.unsupported_grant_type! + end end - def retrieveClient(req, user) - user.o_auth_applications.find_by_client_id req.client_id + def handlePasswordFlow(req, res) + user = User.find_for_database_authentication(username: req.username) + if user + if user.valid_password?(req.password) + res.access_token = user.tokens.create!.bearer_token + else + req.invalid_grant! + end + else + req.invalid_grant! # TODO: Change to user login: Perhaps redirect_to login_path? + end end - def createClient(req, user) - user.o_auth_applications.create!(client_id: req.client_id, client_secret: req.client_secret) + def retrieveClient(req) + OAuthApplication.find_by_client_id req.client_id + end + + def isAppValid(o_auth_app, req) + o_auth_app.client_secret == req.client_secret end end end diff --git a/spec/controllers/openid_connect/clients_controller_spec.rb b/spec/controllers/openid_connect/clients_controller_spec.rb new file mode 100644 index 000000000..bce1f9295 --- /dev/null +++ b/spec/controllers/openid_connect/clients_controller_spec.rb @@ -0,0 +1,23 @@ +require 'spec_helper' + +describe OpenidConnect::ClientsController, type: :controller do + describe "#create" do + context "when valid parameters are passed" do + it "should return a client id" do + post :create, + { + redirect_uris: ["http://localhost"] + } + clientJSON = JSON.parse(response.body) + expect(clientJSON["o_auth_application"]["client_id"].length).to eq(32) + end + end + context "when redirect uri is missing" do + it "should return a invalid_client_metadata error" do + post :create + clientJSON = JSON.parse(response.body) + expect(clientJSON["error"]).to have_content("invalid_client_metadata") + end + end + end +end diff --git a/spec/controllers/api/v0/users_controller_spec.rb b/spec/lib/openid_connect/protected_resource_endpoint_spec.rb similarity index 63% rename from spec/controllers/api/v0/users_controller_spec.rb rename to spec/lib/openid_connect/protected_resource_endpoint_spec.rb index caa874f54..5a8251b0a 100644 --- a/spec/controllers/api/v0/users_controller_spec.rb +++ b/spec/lib/openid_connect/protected_resource_endpoint_spec.rb @@ -1,20 +1,26 @@ require 'spec_helper' -describe Api::V0::UsersController, type: :request do - describe "#show" do - let!(:application) { bob.o_auth_applications.create!(client_id: 1, client_secret: "secret") } - let!(:token) { application.tokens.create!.bearer_token.to_s } +describe OpenidConnect::ProtectedResourceEndpoint, type: :request do + describe "getting the user info" do + let!(:token) { bob.tokens.create!.bearer_token.to_s } let(:invalid_token) { SecureRandom.hex(32).to_s } + # TODO: Add tests for expired access tokens - context "when valid" do + context "when access token is valid" do it "shows the user's username and email" do - get "/api/v0/user/?access_token=" + token + get "/api/v0/user/", + { + access_token: token + } jsonBody = JSON.parse(response.body) expect(jsonBody["username"]).to eq(bob.username) expect(jsonBody["email"]).to eq(bob.email) end it "should include private in the cache-control header" do - get "/api/v0/user/?access_token=" + token + get "/api/v0/user/", + { + access_token: token + } expect(response.headers["Cache-Control"]).to include("private") end end @@ -32,15 +38,24 @@ describe Api::V0::UsersController, type: :request do context "when an invalid access token is provided" do it "should respond with a 401 Unauthorized response" do - get "/api/v0/user/?access_token=" + invalid_token + get "/api/v0/user/", + { + access_token: invalid_token + } expect(response.status).to be(401) end it "should have an auth-scheme value of Bearer" do - get "/api/v0/user/?access_token=" + invalid_token + get "/api/v0/user/", + { + access_token: invalid_token + } expect(response.headers["WWW-Authenticate"]).to include("Bearer") end it "should contain an invalid_token error" do - get "/api/v0/user/?access_token=" + invalid_token + get "/api/v0/user/", + { + access_token: invalid_token + } expect(response.body).to include("invalid_token") end end diff --git a/spec/lib/openid_connect/token_endpoint_spec.rb b/spec/lib/openid_connect/token_endpoint_spec.rb index 4325a05e0..01de06161 100644 --- a/spec/lib/openid_connect/token_endpoint_spec.rb +++ b/spec/lib/openid_connect/token_endpoint_spec.rb @@ -1,63 +1,114 @@ require 'spec_helper' describe OpenidConnect::TokenEndpoint, type: :request do - describe "password grant type" do + let!(:client) { OAuthApplication.create!(redirect_uris: ["http://localhost"]) } + describe "the password grant type" do context "when the username field is missing" do it "should return an invalid request error" do - post "/openid/access_tokens?grant_type=password\&password=bluepin7\&client_id=4\&client_secret=azerty" + post "/openid_connect/access_tokens", + { + grant_type: "password", + password: "bluepin7", + client_id: client.client_id, + client_secret: client.client_secret + } expect(response.body).to include("'username' required") end end context "when the password field is missing" do it "should return an invalid request error" do - post "/openid/access_tokens?grant_type=password\&username=bob\&client_id=4\&client_secret=azerty" + post "/openid_connect/access_tokens", + { + grant_type: "password", + username: "bob", + client_id: client.client_id, + client_secret: client.client_secret + } expect(response.body).to include("'password' required") end end context "when the username does not match an existing user" do it "should return an invalid request error" do - post "/openid/access_tokens?grant_type=password\&username=mewasdfrandom\&password=bluepin7\&client_id=4\&client_secret=azerty" + post "/openid_connect/access_tokens", + { + grant_type: "password", + username: "randomnoexist", + password: "bluepin7", + client_id: client.client_id, + client_secret: client.client_secret + } expect(response.body).to include("invalid_grant") end end context "when the password is invalid" do it "should return an invalid request error" do - post "/openid/access_tokens?grant_type=password\&username=mewasdfrandom\&password=bluepin7\&client_id=4\&client_secret=azerty" + post "/openid_connect/access_tokens", + { + grant_type: "password", + username: "bob", + password: "wrongpassword", + client_id: client.client_id, + client_secret: client.client_secret + } expect(response.body).to include("invalid_grant") end end - context "when there are duplicate fields" do - it "should return an invalid request error" do - post "/openid/access_tokens?grant_type=password\&username=bob\&password=bluepin6\&username=bob\&password=bluepin7\&client_id=4\&client_secret=azerty" - expect(response.body).to include("invalid_grant") - # TODO: Apparently Nov's implementation lets this one pass; however, according to the OIDC spec, we are supposed to reject duplicate fields. Is this a security issue? - end - end - context "when the client is unauthorized" do - # TODO: If we support password grant, we should prevent access from unauthorized client applications - it "should return an error" do - fail - end - end - context "when many unauthorized requests are made" do - # TODO: If we support password grant, we should support a way to prevent brute force attacks (using rate-limitation or generating alerts) as specified by RFC 6749 4.3.2 Access Token Request - it "should generate an alert" do - fail - end - end context "when the request is valid" do it "should return an access token" do - post "/openid/access_tokens?grant_type=password\&username=bob\&password=bluepin7\&client_id=4\&client_secret=azerty" + post "/openid_connect/access_tokens", + { + grant_type: "password", + username: "bob", + password: "bluepin7", + client_id: client.client_id, + client_secret: client.client_secret + } json = JSON.parse(response.body) expect(json["access_token"].length).to eq(64) expect(json["token_type"]).to eq("bearer") expect(json.keys).to include("expires_in") end end + context "when there are duplicate fields" do + it "should return an invalid request error" do + post "/openid_connect/access_tokens", + { + grant_type: "password", + username: "bob", + password: "bluepin7", + username: "bob", + password: "bluepin6", + client_id: client.client_id, + client_secret: client.client_secret + } + expect(response.body).to include("invalid_grant") + end + end + context "when the client is unregistered" do + it "should return an error" do + post "/openid_connect/access_tokens", + { + grant_type: "password", + username: "bob", + password: "bluepin7", + client_id: SecureRandom.hex(16).to_s, + client_secret: client.client_secret + } + expect(response.body).to include("invalid_client") + end + end + # TODO: Support a way to prevent brute force attacks using rate-limitation? as specified by RFC 6749 4.3.2 Access Token Request end - describe "unsupported grant type" do + describe "an unsupported grant type" do it "should return an unsupported grant type error" do - post "/openid/access_tokens?grant_type=me\&username=bob\&password=bluepin7\&client_id=4\&client_secret=azerty" + post "/openid_connect/access_tokens", + { + grant_type: "noexistgrant", + username: "bob", + password: "bluepin7", + client_id: client.client_id, + client_secret: client.client_secret + } expect(response.body).to include "unsupported_grant_type" end end