diff --git a/app/controllers/api/openid_connect/authorizations_controller.rb b/app/controllers/api/openid_connect/authorizations_controller.rb index e31d001bd..5e6cc407c 100644 --- a/app/controllers/api/openid_connect/authorizations_controller.rb +++ b/app/controllers/api/openid_connect/authorizations_controller.rb @@ -122,7 +122,7 @@ module Api end def scopes_as_space_seperated_values - @scopes.map(&:name).join(" ") + @scopes.join(" ") end def process_authorization_consent(approvedString) diff --git a/app/controllers/api/openid_connect/discovery_controller.rb b/app/controllers/api/openid_connect/discovery_controller.rb index c6c8f6cc1..ca8dd18cf 100644 --- a/app/controllers/api/openid_connect/discovery_controller.rb +++ b/app/controllers/api/openid_connect/discovery_controller.rb @@ -19,8 +19,8 @@ module Api authorization_endpoint: new_api_openid_connect_authorization_url, token_endpoint: api_openid_connect_access_tokens_url, userinfo_endpoint: api_openid_connect_user_info_url, - jwks_uri: File.join(root_url, "api", "openid_connect", "jwks.json"), - scopes_supported: Api::OpenidConnect::Scope.pluck(:name), + jwks_uri: api_openid_connect_url, + scopes_supported: %w(openid read write), response_types_supported: Api::OpenidConnect::OAuthApplication.available_response_types, request_object_signing_alg_values_supported: %i(HS256 HS384 HS512), subject_types_supported: %w(public pairwise), diff --git a/app/controllers/api/openid_connect/user_info_controller.rb b/app/controllers/api/openid_connect/user_info_controller.rb index 62de2389b..76b063ba5 100644 --- a/app/controllers/api/openid_connect/user_info_controller.rb +++ b/app/controllers/api/openid_connect/user_info_controller.rb @@ -4,7 +4,7 @@ module Api include Api::OpenidConnect::ProtectedResourceEndpoint before_action do - require_access_token Api::OpenidConnect::Scope.find_by(name: "openid") + require_access_token ["openid"] end def show diff --git a/app/models/api/openid_connect/authorization.rb b/app/models/api/openid_connect/authorization.rb index fce5220fe..a5a9ab3ed 100644 --- a/app/models/api/openid_connect/authorization.rb +++ b/app/models/api/openid_connect/authorization.rb @@ -7,9 +7,9 @@ module Api validates :user, presence: true validates :o_auth_application, presence: true validates :user, uniqueness: {scope: :o_auth_application} + validate :validate_scope_names + serialize :scopes, JSON - has_many :authorization_scopes - has_many :scopes, through: :authorization_scopes has_many :o_auth_access_tokens, dependent: :destroy has_many :id_tokens, dependent: :destroy @@ -21,21 +21,28 @@ module Api self.refresh_token = SecureRandom.hex(32) end - def accessible?(required_scopes=nil) - Array(required_scopes).all? do |required_scope| - scopes.include? required_scope + def validate_scope_names + return unless scopes + scopes.each do |scope| + errors.add(:scope, "is not a valid scope name") unless %w(openid read write).include? scope end end + def accessible?(required_scopes=nil) + Array(required_scopes).all? { |required_scope| + scopes.include? required_scope + } + end + def create_code - self.code = SecureRandom.hex(32) - save - code + SecureRandom.hex(32).tap do |code| + self.code = code + save + end end def create_access_token o_auth_access_tokens.create!.bearer_token - # TODO: Add support for request object end def create_id_token @@ -53,9 +60,12 @@ module Api end def self.use_code(code) - auth = find_by(code: code) - auth.code = nil if auth # Remove auth code if found so it can't be reused - auth + return unless code + find_by(code: code).tap do |auth| + return unless auth + auth.code = nil + auth.save + end end end end diff --git a/app/presenters/user_applications_presenter.rb b/app/presenters/user_applications_presenter.rb index 8c2d263fd..18cb63480 100644 --- a/app/presenters/user_applications_presenter.rb +++ b/app/presenters/user_applications_presenter.rb @@ -4,6 +4,7 @@ class UserApplicationsPresenter end def user_applications + # TODO: Fix and add tests @applications ||= @current_user.o_auth_applications.each_with_object([]) do |app, array| array << app_as_json(app) end @@ -29,9 +30,8 @@ class UserApplicationsPresenter end def find_scopes(application) - scopes = Api::OpenidConnect::Authorization.find_by_client_id_and_user( + Api::OpenidConnect::Authorization.find_by_client_id_and_user( application.client_id, @current_user).scopes - scopes.each_with_object([]) {|scope, array| array << scope.name } end def find_id(application) diff --git a/app/views/api/openid_connect/authorizations/new.html.haml b/app/views/api/openid_connect/authorizations/new.html.haml index 8009785ba..a20a3863b 100644 --- a/app/views/api/openid_connect/authorizations/new.html.haml +++ b/app/views/api/openid_connect/authorizations/new.html.haml @@ -4,7 +4,7 @@ = t(".with_id_token") %ul - @scopes.each do |scope| - %li= scope.name + %li= scope - if @request_object %li= t(".requested_objects") %ul diff --git a/db/migrate/20150708153926_create_authorizations.rb b/db/migrate/20150708153926_create_authorizations.rb index af659bcf0..07fc13002 100644 --- a/db/migrate/20150708153926_create_authorizations.rb +++ b/db/migrate/20150708153926_create_authorizations.rb @@ -7,6 +7,7 @@ class CreateAuthorizations < ActiveRecord::Migration t.string :code t.string :redirect_uri t.string :nonce + t.string :scopes t.timestamps null: false end diff --git a/db/migrate/20150708154727_create_scope.rb b/db/migrate/20150708154727_create_scope.rb deleted file mode 100644 index afdc320f6..000000000 --- a/db/migrate/20150708154727_create_scope.rb +++ /dev/null @@ -1,9 +0,0 @@ -class CreateScope < ActiveRecord::Migration - def change - create_table :scopes do |t| - t.primary_key :name, :string - - t.timestamps null: false - end - end -end diff --git a/db/migrate/20150708155202_create_authorization_scopes_join_table.rb b/db/migrate/20150708155202_create_authorization_scopes_join_table.rb deleted file mode 100644 index 79c275b41..000000000 --- a/db/migrate/20150708155202_create_authorization_scopes_join_table.rb +++ /dev/null @@ -1,8 +0,0 @@ -class CreateAuthorizationScopesJoinTable < ActiveRecord::Migration - def change - create_table :authorization_scopes, id: false do |t| - t.belongs_to :authorization, index: true - t.belongs_to :scope, index: true - end - end -end diff --git a/db/schema.rb b/db/schema.rb index fa60983f8..ac983f750 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -55,14 +55,6 @@ ActiveRecord::Schema.define(version: 20150801074555) do add_index "aspects", ["user_id", "contacts_visible"], name: "index_aspects_on_user_id_and_contacts_visible", using: :btree add_index "aspects", ["user_id"], name: "index_aspects_on_user_id", using: :btree - create_table "authorization_scopes", id: false, force: :cascade do |t| - t.integer "authorization_id", limit: 4 - t.integer "scope_id", limit: 4 - end - - add_index "authorization_scopes", ["authorization_id"], name: "index_authorization_scopes_on_authorization_id", using: :btree - add_index "authorization_scopes", ["scope_id"], name: "index_authorization_scopes_on_scope_id", using: :btree - create_table "authorizations", force: :cascade do |t| t.integer "user_id", limit: 4 t.integer "o_auth_application_id", limit: 4 @@ -70,6 +62,7 @@ ActiveRecord::Schema.define(version: 20150801074555) do t.string "code", limit: 255 t.string "redirect_uri", limit: 255 t.string "nonce", limit: 255 + t.string "scopes", limit: 255 t.datetime "created_at", null: false t.datetime "updated_at", null: false end @@ -532,12 +525,6 @@ ActiveRecord::Schema.define(version: 20150801074555) do t.datetime "updated_at", null: false end - create_table "scopes", force: :cascade do |t| - t.string "name", limit: 255 - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - end - create_table "services", force: :cascade do |t| t.string "type", limit: 127, null: false t.integer "user_id", limit: 4, null: false diff --git a/db/seeds.rb b/db/seeds.rb deleted file mode 100644 index 6ca70e345..000000000 --- a/db/seeds.rb +++ /dev/null @@ -1,3 +0,0 @@ -Api::OpenidConnect::Scope.find_or_create_by!(name: "openid") -Api::OpenidConnect::Scope.find_or_create_by!(name: "read") -Api::OpenidConnect::Scope.find_or_create_by!(name: "write") diff --git a/features/desktop/oidc_auth_code_flow.feature b/features/desktop/oidc_auth_code_flow.feature index 49dd10fe6..7a25bca72 100644 --- a/features/desktop/oidc_auth_code_flow.feature +++ b/features/desktop/oidc_auth_code_flow.feature @@ -2,7 +2,6 @@ Feature: Access protected resources using auth code flow Background: Given a user with username "kent" - And all scopes exist Scenario: Invalid client id to auth endpoint When I register a new client diff --git a/features/desktop/oidc_implicit_flow.feature b/features/desktop/oidc_implicit_flow.feature index 8ded34fd3..6d02919c1 100644 --- a/features/desktop/oidc_implicit_flow.feature +++ b/features/desktop/oidc_implicit_flow.feature @@ -2,7 +2,6 @@ Feature: Access protected resources using implicit flow Background: Given a user with username "kent" - And all scopes exist Scenario: Invalid client id to auth endpoint When I register a new client diff --git a/features/step_definitions/oidc_common_steps.rb b/features/step_definitions/oidc_common_steps.rb index f73d4407b..5c562f891 100644 --- a/features/step_definitions/oidc_common_steps.rb +++ b/features/step_definitions/oidc_common_steps.rb @@ -1,8 +1,3 @@ -Given(/^all scopes exist$/) do - Api::OpenidConnect::Scope.find_or_create_by(name: "openid") - Api::OpenidConnect::Scope.find_or_create_by(name: "read") -end - Given /^a client with a provided picture exists for user "([^\"]*)"$/ do |email| app = FactoryGirl.create(:o_auth_application_with_image) user = User.find_by(email: email) diff --git a/lib/api/openid_connect/authorization_point/endpoint.rb b/lib/api/openid_connect/authorization_point/endpoint.rb index 38ccb5f99..053686e92 100644 --- a/lib/api/openid_connect/authorization_point/endpoint.rb +++ b/lib/api/openid_connect/authorization_point/endpoint.rb @@ -44,9 +44,10 @@ module Api end def build_scopes(req) - @scopes = req.scope.map {|scope_name| - OpenidConnect::Scope.where(name: scope_name).first.tap do |scope| - req.invalid_scope! "Unknown scope: #{scope}" unless scope + @scopes = req.scope.map {|scope| + scope.tap do |scope_name| + # TODO: Use enum + req.invalid_scope! "Unknown scope: #{scope_name}" unless %w(openid read write).include? scope_name end } end diff --git a/lib/api/openid_connect/authorization_point/endpoint_confirmation_point.rb b/lib/api/openid_connect/authorization_point/endpoint_confirmation_point.rb index 104b0a0fb..6a3670f8c 100644 --- a/lib/api/openid_connect/authorization_point/endpoint_confirmation_point.rb +++ b/lib/api/openid_connect/authorization_point/endpoint_confirmation_point.rb @@ -19,16 +19,23 @@ module Api end end - # TODO: Add support for request object + private + def approved!(req, res) - auth = OpenidConnect::Authorization.find_or_create_by( - o_auth_application: @o_auth_application, user: @user, redirect_uri: @redirect_uri) - auth.nonce = req.nonce - auth.scopes << @scopes unless auth.scopes == @scopes + auth = find_or_build_auth(req) handle_approved_response_type(auth, req, res) res.approve! end + def find_or_build_auth(req) + OpenidConnect::Authorization.find_or_create_by!( + o_auth_application: @o_auth_application, user: @user, redirect_uri: @redirect_uri).tap do |auth| + auth.nonce = req.nonce + auth.scopes = @scopes + auth.save + end + end + def handle_approved_response_type(auth, req, res) response_types = Array(req.response_type) handle_approved_auth_code(auth, res, response_types) diff --git a/lib/api/openid_connect/protected_resource_endpoint.rb b/lib/api/openid_connect/protected_resource_endpoint.rb index ca9f82ffe..a64c5db45 100644 --- a/lib/api/openid_connect/protected_resource_endpoint.rb +++ b/lib/api/openid_connect/protected_resource_endpoint.rb @@ -3,7 +3,7 @@ module Api module ProtectedResourceEndpoint attr_reader :current_token - def require_access_token(*required_scopes) + def require_access_token(required_scopes) @current_token = request.env[Rack::OAuth2::Server::Resource::ACCESS_TOKEN] raise Rack::OAuth2::Server::Resource::Bearer::Unauthorized.new("Unauthorized user") unless @current_token && @current_token.authorization diff --git a/lib/api/openid_connect/token_endpoint.rb b/lib/api/openid_connect/token_endpoint.rb index d71525e87..627dc6737 100644 --- a/lib/api/openid_connect/token_endpoint.rb +++ b/lib/api/openid_connect/token_endpoint.rb @@ -23,7 +23,7 @@ module Api auth = Api::OpenidConnect::Authorization.with_redirect_uri(req.redirect_uri).use_code(req.code) req.invalid_grant! if auth.blank? res.access_token = auth.create_access_token - if auth.accessible?(Api::OpenidConnect::Scope.find_by!(name: "openid")) + if auth.accessible? "openid" id_token = auth.create_id_token res.id_token = id_token.to_jwt(access_token: res.access_token) end @@ -32,16 +32,6 @@ module Api end end - def build_auth_and_access_token(auth, req, res) - scope_list = req.scope.map { |scope_name| - OpenidConnect::Scope.find_by(name: scope_name).tap do |scope| - req.invalid_scope! "Unknown scope: #{scope}" unless scope - end - } # TODO: Check client scope permissions - auth.scopes << scope_list - res.access_token = auth.create_access_token - end - def handle_refresh_flow(req, res) # Handle as if scope request was omitted even if provided. # See https://tools.ietf.org/html/rfc6749#section-6 for handling diff --git a/spec/controllers/api/openid_connect/authorizations_controller_spec.rb b/spec/controllers/api/openid_connect/authorizations_controller_spec.rb index 5233ef976..fe81b3242 100644 --- a/spec/controllers/api/openid_connect/authorizations_controller_spec.rb +++ b/spec/controllers/api/openid_connect/authorizations_controller_spec.rb @@ -133,7 +133,7 @@ describe Api::OpenidConnect::AuthorizationsController, type: :controller do context "when already authorized" do let!(:auth) { Api::OpenidConnect::Authorization.find_or_create_by(o_auth_application: client, user: alice, - redirect_uri: "http://localhost:3000/") + redirect_uri: "http://localhost:3000/", scopes: ["openid"]) } context "when valid parameters are passed" do diff --git a/spec/factories.rb b/spec/factories.rb index ce54a4aa4..772d36d43 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -336,32 +336,19 @@ FactoryGirl.define do factory :auth_with_read, class: Api::OpenidConnect::Authorization do o_auth_application user - - after(:create) do |auth_with_read| - auth_with_read.scopes << [Api::OpenidConnect::Scope.find_or_create_by(name: "openid"), - Api::OpenidConnect::Scope.find_or_create_by(name: "read")] - end + scopes ["openid","read"] end factory :auth_with_read_and_ppid, class: Api::OpenidConnect::Authorization do association :o_auth_application, factory: :o_auth_application_with_ppid user - - after(:create) do |auth_with_read| - auth_with_read.scopes << [Api::OpenidConnect::Scope.find_or_create_by(name: "openid"), - Api::OpenidConnect::Scope.find_or_create_by(name: "read")] - end + scopes ["openid","read"] end factory :auth_with_read_and_write, class: Api::OpenidConnect::Authorization do o_auth_application user - - after(:create) do |auth_with_read| - auth_with_read.scopes << [Api::OpenidConnect::Scope.find_or_create_by(name: "openid"), - Api::OpenidConnect::Scope.find_or_create_by(name: "read"), - Api::OpenidConnect::Scope.find_or_create_by(name: "write")] - end + scopes ["openid","read","write"] end # Factories for the DiasporaFederation-gem diff --git a/spec/lib/api/openid_connect/token_endpoint_spec.rb b/spec/lib/api/openid_connect/token_endpoint_spec.rb index dfbee1eeb..5f044d9ae 100644 --- a/spec/lib/api/openid_connect/token_endpoint_spec.rb +++ b/spec/lib/api/openid_connect/token_endpoint_spec.rb @@ -1,12 +1,10 @@ require "spec_helper" describe Api::OpenidConnect::TokenEndpoint, type: :request do let!(:client) { FactoryGirl.create(:o_auth_application_with_ppid) } - let!(:auth) do - auth = Api::OpenidConnect::Authorization.find_or_create_by( - o_auth_application: client, user: bob, redirect_uri: "http://localhost:3000/") - auth.scopes << [Api::OpenidConnect::Scope.find_by!(name: "openid")] - auth - end + let!(:auth) { + Api::OpenidConnect::Authorization.find_or_create_by( + o_auth_application: client, user: bob, redirect_uri: "http://localhost:3000/", scopes: ["openid"]) + } let!(:code) { auth.create_code } describe "the authorization code grant type" do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index c0a67974d..ca444256a 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -84,7 +84,6 @@ RSpec.configure do |config| $process_queue = false allow_any_instance_of(Postzord::Dispatcher::Public).to receive(:deliver_to_remote) allow_any_instance_of(Postzord::Dispatcher::Private).to receive(:deliver_to_remote) - load "#{Rails.root}/db/seeds.rb" end config.expect_with :rspec do |expect_config|