From 8a22bfb222b3b063a03efd5e85da834f3d63b5fc Mon Sep 17 00:00:00 2001 From: Milan Cvetkovic Date: Wed, 30 Aug 2023 12:19:00 +0000 Subject: [PATCH 1/6] Add unconfigured doorkeeper-openid_connect After executing: rails generate doorkeeper:openid_connect:install rails generate doorkeeper:openid_connect:install Split migration script to 2 to avoid deadlock. --- Gemfile | 1 + Gemfile.lock | 4 ++ .../initializers/doorkeeper_openid_connect.rb | 72 +++++++++++++++++++ .../locales/doorkeeper_openid_connect.en.yml | 23 ++++++ config/routes.rb | 1 + ...create_doorkeeper_openid_connect_tables.rb | 18 +++++ ...create_doorkeeper_openid_connect_tables.rb | 6 ++ db/structure.sql | 62 ++++++++++++++++ 8 files changed, 187 insertions(+) create mode 100644 config/initializers/doorkeeper_openid_connect.rb create mode 100644 config/locales/doorkeeper_openid_connect.en.yml create mode 100644 db/migrate/20230830115219_create_doorkeeper_openid_connect_tables.rb create mode 100644 db/migrate/20230830115220_validate_create_doorkeeper_openid_connect_tables.rb diff --git a/Gemfile b/Gemfile index f60a219e7..bf65965d4 100644 --- a/Gemfile +++ b/Gemfile @@ -79,6 +79,7 @@ gem "omniauth-rails_csrf_protection", "~> 1.0" # Doorkeeper for OAuth2 gem "doorkeeper" gem "doorkeeper-i18n" +gem "doorkeeper-openid_connect" # Markdown formatting support gem "kramdown" diff --git a/Gemfile.lock b/Gemfile.lock index 5f36ab055..2a51a3d36 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -166,6 +166,9 @@ GEM railties (>= 5) doorkeeper-i18n (5.2.6) doorkeeper (>= 5.2) + doorkeeper-openid_connect (1.8.7) + doorkeeper (>= 5.5, < 5.7) + jwt (>= 2.5) dry-configurable (1.1.0) dry-core (~> 1.0, < 2) zeitwerk (~> 2.6) @@ -567,6 +570,7 @@ DEPENDENCIES delayed_job_active_record doorkeeper doorkeeper-i18n + doorkeeper-openid_connect erb_lint factory_bot_rails faraday diff --git a/config/initializers/doorkeeper_openid_connect.rb b/config/initializers/doorkeeper_openid_connect.rb new file mode 100644 index 000000000..e91a907c2 --- /dev/null +++ b/config/initializers/doorkeeper_openid_connect.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +Doorkeeper::OpenidConnect.configure do + issuer do |_resource_owner, _application| + "issuer string" + end + + signing_key <<~KEY + -----BEGIN RSA PRIVATE KEY----- + .... + -----END RSA PRIVATE KEY----- + KEY + + subject_types_supported [:public] + + resource_owner_from_access_token do |access_token| + # Example implementation: + # User.find_by(id: access_token.resource_owner_id) + end + + auth_time_from_resource_owner do |resource_owner| + # Example implementation: + # resource_owner.current_sign_in_at + end + + reauthenticate_resource_owner do |resource_owner, return_to| + # Example implementation: + # store_location_for resource_owner, return_to + # sign_out resource_owner + # redirect_to new_user_session_url + end + + # Depending on your configuration, a DoubleRenderError could be raised + # if render/redirect_to is called at some point before this callback is executed. + # To avoid the DoubleRenderError, you could add these two lines at the beginning + # of this callback: (Reference: https://github.com/rails/rails/issues/25106) + # self.response_body = nil + # @_response_body = nil + select_account_for_resource_owner do |resource_owner, return_to| + # Example implementation: + # store_location_for resource_owner, return_to + # redirect_to account_select_url + end + + subject do |resource_owner, application| + # Example implementation: + # resource_owner.id + + # or if you need pairwise subject identifier, implement like below: + # Digest::SHA256.hexdigest("#{resource_owner.id}#{URI.parse(application.redirect_uri).host}#{'your_secret_salt'}") + end + + # Protocol to use when generating URIs for the discovery endpoint, + # for example if you also use HTTPS in development + # protocol do + # :https + # end + + # Expiration time on or after which the ID Token MUST NOT be accepted for processing. (default 120 seconds). + # expiration 600 + + # Example claims: + # claims do + # normal_claim :_foo_ do |resource_owner| + # resource_owner.foo + # end + + # normal_claim :_bar_ do |resource_owner| + # resource_owner.bar + # end + # end +end diff --git a/config/locales/doorkeeper_openid_connect.en.yml b/config/locales/doorkeeper_openid_connect.en.yml new file mode 100644 index 000000000..1bed506b2 --- /dev/null +++ b/config/locales/doorkeeper_openid_connect.en.yml @@ -0,0 +1,23 @@ +en: + doorkeeper: + scopes: + openid: 'Authenticate your account' + profile: 'View your profile information' + email: 'View your email address' + address: 'View your physical address' + phone: 'View your phone number' + errors: + messages: + login_required: 'The authorization server requires end-user authentication' + consent_required: 'The authorization server requires end-user consent' + interaction_required: 'The authorization server requires end-user interaction' + account_selection_required: 'The authorization server requires end-user account selection' + openid_connect: + errors: + messages: + # Configuration error messages + resource_owner_from_access_token_not_configured: 'Failure due to Doorkeeper::OpenidConnect.configure.resource_owner_from_access_token missing configuration.' + auth_time_from_resource_owner_not_configured: 'Failure due to Doorkeeper::OpenidConnect.configure.auth_time_from_resource_owner missing configuration.' + reauthenticate_resource_owner_not_configured: 'Failure due to Doorkeeper::OpenidConnect.configure.reauthenticate_resource_owner missing configuration.' + select_account_for_resource_owner_not_configured: 'Failure due to Doorkeeper::OpenidConnect.configure.select_account_for_resource_owner missing configuration.' + subject_not_configured: 'ID Token generation failed due to Doorkeeper::OpenidConnect.configure.subject missing configuration.' diff --git a/config/routes.rb b/config/routes.rb index 404e7b0a3..5b537ea3e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,4 +1,5 @@ OpenStreetMap::Application.routes.draw do + use_doorkeeper_openid_connect use_doorkeeper :scope => "oauth2" do controllers :authorizations => "oauth2_authorizations", :applications => "oauth2_applications", diff --git a/db/migrate/20230830115219_create_doorkeeper_openid_connect_tables.rb b/db/migrate/20230830115219_create_doorkeeper_openid_connect_tables.rb new file mode 100644 index 000000000..4924e158d --- /dev/null +++ b/db/migrate/20230830115219_create_doorkeeper_openid_connect_tables.rb @@ -0,0 +1,18 @@ +class CreateDoorkeeperOpenidConnectTables < ActiveRecord::Migration[7.0] + def change + create_table :oauth_openid_requests do |t| + t.references :access_grant, :null => false, :index => true + t.string :nonce, :null => false + end + + # Avoid validating foreign keys doe to possible deadlock + # create a separate migration instead, as suggested by db:migrate + + add_foreign_key( + :oauth_openid_requests, + :oauth_access_grants, + :column => :access_grant_id, + :on_delete => :cascade, :validate => false + ) + end +end diff --git a/db/migrate/20230830115220_validate_create_doorkeeper_openid_connect_tables.rb b/db/migrate/20230830115220_validate_create_doorkeeper_openid_connect_tables.rb new file mode 100644 index 000000000..0596cbeb1 --- /dev/null +++ b/db/migrate/20230830115220_validate_create_doorkeeper_openid_connect_tables.rb @@ -0,0 +1,6 @@ +class ValidateCreateDoorkeeperOpenidConnectTables < ActiveRecord::Migration[7.0] + # Validate foreign key created by CreateDoorkeeperOpenidConnectTables + def change + validate_foreign_key :oauth_openid_requests, :oauth_access_grants + end +end diff --git a/db/structure.sql b/db/structure.sql index 1874e6461..bd65755f2 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -1152,6 +1152,36 @@ CREATE SEQUENCE public.oauth_nonces_id_seq ALTER SEQUENCE public.oauth_nonces_id_seq OWNED BY public.oauth_nonces.id; +-- +-- Name: oauth_openid_requests; Type: TABLE; Schema: public; Owner: - +-- + +CREATE TABLE public.oauth_openid_requests ( + id bigint NOT NULL, + access_grant_id bigint NOT NULL, + nonce character varying NOT NULL +); + + +-- +-- Name: oauth_openid_requests_id_seq; Type: SEQUENCE; Schema: public; Owner: - +-- + +CREATE SEQUENCE public.oauth_openid_requests_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + + +-- +-- Name: oauth_openid_requests_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: - +-- + +ALTER SEQUENCE public.oauth_openid_requests_id_seq OWNED BY public.oauth_openid_requests.id; + + -- -- Name: oauth_tokens; Type: TABLE; Schema: public; Owner: - -- @@ -1704,6 +1734,13 @@ ALTER TABLE ONLY public.oauth_applications ALTER COLUMN id SET DEFAULT nextval(' ALTER TABLE ONLY public.oauth_nonces ALTER COLUMN id SET DEFAULT nextval('public.oauth_nonces_id_seq'::regclass); +-- +-- Name: oauth_openid_requests id; Type: DEFAULT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public.oauth_openid_requests ALTER COLUMN id SET DEFAULT nextval('public.oauth_openid_requests_id_seq'::regclass); + + -- -- Name: oauth_tokens id; Type: DEFAULT; Schema: public; Owner: - -- @@ -2033,6 +2070,14 @@ ALTER TABLE ONLY public.oauth_nonces ADD CONSTRAINT oauth_nonces_pkey PRIMARY KEY (id); +-- +-- Name: oauth_openid_requests oauth_openid_requests_pkey; Type: CONSTRAINT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public.oauth_openid_requests + ADD CONSTRAINT oauth_openid_requests_pkey PRIMARY KEY (id); + + -- -- Name: oauth_tokens oauth_tokens_pkey; Type: CONSTRAINT; Schema: public; Owner: - -- @@ -2573,6 +2618,13 @@ CREATE UNIQUE INDEX index_oauth_applications_on_uid ON public.oauth_applications CREATE UNIQUE INDEX index_oauth_nonces_on_nonce_and_timestamp ON public.oauth_nonces USING btree (nonce, "timestamp"); +-- +-- Name: index_oauth_openid_requests_on_access_grant_id; Type: INDEX; Schema: public; Owner: - +-- + +CREATE INDEX index_oauth_openid_requests_on_access_grant_id ON public.oauth_openid_requests USING btree (access_grant_id); + + -- -- Name: index_oauth_tokens_on_token; Type: INDEX; Schema: public; Owner: - -- @@ -2989,6 +3041,14 @@ ALTER TABLE ONLY public.oauth_access_tokens ADD CONSTRAINT fk_rails_732cb83ab7 FOREIGN KEY (application_id) REFERENCES public.oauth_applications(id) NOT VALID; +-- +-- Name: oauth_openid_requests fk_rails_77114b3b09; Type: FK CONSTRAINT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public.oauth_openid_requests + ADD CONSTRAINT fk_rails_77114b3b09 FOREIGN KEY (access_grant_id) REFERENCES public.oauth_access_grants(id) ON DELETE CASCADE; + + -- -- Name: active_storage_variant_records fk_rails_993965df05; Type: FK CONSTRAINT; Schema: public; Owner: - -- @@ -3404,6 +3464,8 @@ INSERT INTO "schema_migrations" (version) VALUES ('20220223140543'), ('20230816135800'), ('20230825162137'), +('20230830115219'), +('20230830115220'), ('21'), ('22'), ('23'), From e996ee5dbca8b7ed26dbf55fcc116fa947f60188 Mon Sep 17 00:00:00 2001 From: Milan Cvetkovic Date: Mon, 4 Sep 2023 14:46:29 +0000 Subject: [PATCH 2/6] Merge locale definitions to config/locales/en.yml --- .../locales/doorkeeper_openid_connect.en.yml | 23 ------------------- config/locales/en.yml | 21 +++++++++++++++++ 2 files changed, 21 insertions(+), 23 deletions(-) delete mode 100644 config/locales/doorkeeper_openid_connect.en.yml diff --git a/config/locales/doorkeeper_openid_connect.en.yml b/config/locales/doorkeeper_openid_connect.en.yml deleted file mode 100644 index 1bed506b2..000000000 --- a/config/locales/doorkeeper_openid_connect.en.yml +++ /dev/null @@ -1,23 +0,0 @@ -en: - doorkeeper: - scopes: - openid: 'Authenticate your account' - profile: 'View your profile information' - email: 'View your email address' - address: 'View your physical address' - phone: 'View your phone number' - errors: - messages: - login_required: 'The authorization server requires end-user authentication' - consent_required: 'The authorization server requires end-user consent' - interaction_required: 'The authorization server requires end-user interaction' - account_selection_required: 'The authorization server requires end-user account selection' - openid_connect: - errors: - messages: - # Configuration error messages - resource_owner_from_access_token_not_configured: 'Failure due to Doorkeeper::OpenidConnect.configure.resource_owner_from_access_token missing configuration.' - auth_time_from_resource_owner_not_configured: 'Failure due to Doorkeeper::OpenidConnect.configure.auth_time_from_resource_owner missing configuration.' - reauthenticate_resource_owner_not_configured: 'Failure due to Doorkeeper::OpenidConnect.configure.reauthenticate_resource_owner missing configuration.' - select_account_for_resource_owner_not_configured: 'Failure due to Doorkeeper::OpenidConnect.configure.select_account_for_resource_owner missing configuration.' - subject_not_configured: 'ID Token generation failed due to Doorkeeper::OpenidConnect.configure.subject missing configuration.' diff --git a/config/locales/en.yml b/config/locales/en.yml index ec845dc7c..c9583abce 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -557,10 +557,31 @@ en: newer_comments: "Newer Comments" older_comments: "Older Comments" doorkeeper: + errors: + messages: + account_selection_required: "The authorization server requires end-user account selection" + consent_required: "The authorization server requires end-user consent" + interaction_required: "The authorization server requires end-user interaction" + login_required: "The authorization server requires end-user authentication" flash: applications: create: notice: Application Registered. + openid_connect: + errors: + messages: + # Configuration error messages + auth_time_from_resource_owner_not_configured: "Failure due to Doorkeeper::OpenidConnect.configure.auth_time_from_resource_owner missing configuration." + reauthenticate_resource_owner_not_configured: "Failure due to Doorkeeper::OpenidConnect.configure.reauthenticate_resource_owner missing configuration." + resource_owner_from_access_token_not_configured: "Failure due to Doorkeeper::OpenidConnect.configure.resource_owner_from_access_token missing configuration." + select_account_for_resource_owner_not_configured: "Failure due to Doorkeeper::OpenidConnect.configure.select_account_for_resource_owner missing configuration." + subject_not_configured: "ID Token generation failed due to Doorkeeper::OpenidConnect.configure.subject missing configuration." + scopes: + address: "View your physical address" + email: "View your email address" + openid: "Authenticate your account" + phone: "View your phone number" + profile: "View your profile information" errors: contact: contact_url: https://wiki.openstreetmap.org/wiki/Contact From 64bcf7652bc1053741ab58f3ff54505b3d7820ad Mon Sep 17 00:00:00 2001 From: Milan Cvetkovic Date: Wed, 30 Aug 2023 12:36:55 +0000 Subject: [PATCH 3/6] Add openid connect support using doorkeeper-openid_connect gem ... as discussed in [Issue 507](https://github.com/openstreetmap/operations/issues/507) and described by @mmd-osm. To activate, set the value of `doorkeeper_signing_key` to RSA private key. Allows using openstreetmap as an identity provider. Adds `openid` scope to OAuth2 authorizations, required to login to OSM. Currently, the only claims returned are: - "openid" scope: "sub" and "preferred_username" - "read_email" scope: "email" --- app/views/oauth2_applications/_form.html.erb | 2 +- app/views/oauth2_authorizations/new.html.erb | 2 + config/initializers/doorkeeper.rb | 2 +- .../initializers/doorkeeper_openid_connect.rb | 66 +++++-------------- config/locales/en.yml | 1 + config/routes.rb | 3 +- config/settings.yml | 5 ++ lib/oauth.rb | 4 +- 8 files changed, 31 insertions(+), 54 deletions(-) diff --git a/app/views/oauth2_applications/_form.html.erb b/app/views/oauth2_applications/_form.html.erb index 7fde3e0e7..51267c069 100644 --- a/app/views/oauth2_applications/_form.html.erb +++ b/app/views/oauth2_applications/_form.html.erb @@ -3,5 +3,5 @@ <%= f.form_group :confidential do %> <%= f.check_box :confidential %> <% end %> -<%= f.collection_check_boxes :scopes, Oauth.scopes(:privileged => current_user.administrator?), :name, :description %> +<%= f.collection_check_boxes :scopes, Oauth.scopes(:oauth2 => true, :privileged => current_user.administrator?), :name, :description %> <%= f.primary %> diff --git a/app/views/oauth2_authorizations/new.html.erb b/app/views/oauth2_authorizations/new.html.erb index 971e0e20a..ac9c7c6c5 100644 --- a/app/views/oauth2_authorizations/new.html.erb +++ b/app/views/oauth2_authorizations/new.html.erb @@ -18,6 +18,7 @@ <%= f.hidden_field :state, :value => @pre_auth.state %> <%= f.hidden_field :response_type, :value => @pre_auth.response_type %> <%= f.hidden_field :scope, :value => @pre_auth.scope %> + <%= f.hidden_field :nonce, :value => @pre_auth.nonce %> <%= f.hidden_field :code_challenge, :value => @pre_auth.code_challenge %> <%= f.hidden_field :code_challenge_method, :value => @pre_auth.code_challenge_method %> <%= f.primary t(".authorize") %> @@ -30,6 +31,7 @@ <%= f.hidden_field :state, :value => @pre_auth.state %> <%= f.hidden_field :response_type, :value => @pre_auth.response_type %> <%= f.hidden_field :scope, :value => @pre_auth.scope %> + <%= f.hidden_field :nonce, :value => @pre_auth.nonce %> <%= f.hidden_field :code_challenge, :value => @pre_auth.code_challenge %> <%= f.hidden_field :code_challenge_method, :value => @pre_auth.code_challenge_method %> <%= f.submit t(".deny") %> diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index a2df9167f..c1d4e2f78 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -225,7 +225,7 @@ Doorkeeper.configure do # https://doorkeeper.gitbook.io/guides/ruby-on-rails/scopes # default_scopes :public - optional_scopes(*Oauth::SCOPES, *Oauth::PRIVILEGED_SCOPES) + optional_scopes(*Oauth::SCOPES, *Oauth::PRIVILEGED_SCOPES, *Oauth::OAUTH2_SCOPES) # Allows to restrict only certain scopes for grant_type. # By default, all the scopes will be available for all the grant types. diff --git a/config/initializers/doorkeeper_openid_connect.rb b/config/initializers/doorkeeper_openid_connect.rb index e91a907c2..7f409ecbe 100644 --- a/config/initializers/doorkeeper_openid_connect.rb +++ b/config/initializers/doorkeeper_openid_connect.rb @@ -2,71 +2,37 @@ Doorkeeper::OpenidConnect.configure do issuer do |_resource_owner, _application| - "issuer string" + "#{Settings.server_protocol}://#{Settings.server_url}" end - signing_key <<~KEY - -----BEGIN RSA PRIVATE KEY----- - .... - -----END RSA PRIVATE KEY----- - KEY + signing_key Settings.doorkeeper_signing_key subject_types_supported [:public] resource_owner_from_access_token do |access_token| - # Example implementation: - # User.find_by(id: access_token.resource_owner_id) + User.find_by(:id => access_token.resource_owner_id) end auth_time_from_resource_owner do |resource_owner| - # Example implementation: - # resource_owner.current_sign_in_at + # empty block necessary as a workaround to missing configuration + # when no auth_time claim is provided end - reauthenticate_resource_owner do |resource_owner, return_to| - # Example implementation: - # store_location_for resource_owner, return_to - # sign_out resource_owner - # redirect_to new_user_session_url + subject do |resource_owner, _application| + resource_owner.id end - # Depending on your configuration, a DoubleRenderError could be raised - # if render/redirect_to is called at some point before this callback is executed. - # To avoid the DoubleRenderError, you could add these two lines at the beginning - # of this callback: (Reference: https://github.com/rails/rails/issues/25106) - # self.response_body = nil - # @_response_body = nil - select_account_for_resource_owner do |resource_owner, return_to| - # Example implementation: - # store_location_for resource_owner, return_to - # redirect_to account_select_url + protocol do + Settings.server_protocol.to_sym end - subject do |resource_owner, application| - # Example implementation: - # resource_owner.id + claims do + claim :preferred_username, :scope => :openid do |resource_owner, _scopes, _access_token| + resource_owner.display_name + end - # or if you need pairwise subject identifier, implement like below: - # Digest::SHA256.hexdigest("#{resource_owner.id}#{URI.parse(application.redirect_uri).host}#{'your_secret_salt'}") + claim :email, :scope => :read_email, :response => [:id_token, :user_info] do |resource_owner, _scopes, _access_token| + resource_owner.email + end end - - # Protocol to use when generating URIs for the discovery endpoint, - # for example if you also use HTTPS in development - # protocol do - # :https - # end - - # Expiration time on or after which the ID Token MUST NOT be accepted for processing. (default 120 seconds). - # expiration 600 - - # Example claims: - # claims do - # normal_claim :_foo_ do |resource_owner| - # resource_owner.foo - # end - - # normal_claim :_bar_ do |resource_owner| - # resource_owner.bar - # end - # end end diff --git a/config/locales/en.yml b/config/locales/en.yml index c9583abce..aca571d53 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2549,6 +2549,7 @@ en: permissions: missing: "You have not permitted the application access to this facility" scopes: + openid: Sign-in using OpenStreetMap read_prefs: Read user preferences write_prefs: Modify user preferences write_diary: Create diary entries, comments and make friends diff --git a/config/routes.rb b/config/routes.rb index 5b537ea3e..43c43a793 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,11 +1,12 @@ OpenStreetMap::Application.routes.draw do - use_doorkeeper_openid_connect use_doorkeeper :scope => "oauth2" do controllers :authorizations => "oauth2_authorizations", :applications => "oauth2_applications", :authorized_applications => "oauth2_authorized_applications" end + use_doorkeeper_openid_connect :scope => "oauth2" if Settings.key?(:doorkeeper_signing_key) + # API namespace :api do get "capabilities" => "capabilities#show" # Deprecated, remove when 0.6 support is removed diff --git a/config/settings.yml b/config/settings.yml index 3ea298efc..f30331b07 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -150,3 +150,8 @@ smtp_password: null #signup_ip_max_burst: #signup_email_per_day: #signup_email_max_burst: +# Private key for signing id_tokens +#doorkeeper_signing_key: | +# -----BEGIN PRIVATE KEY----- +# ... +# -----END PRIVATE KEY----- diff --git a/lib/oauth.rb b/lib/oauth.rb index 7ff2ba8b4..0456c0873 100644 --- a/lib/oauth.rb +++ b/lib/oauth.rb @@ -1,6 +1,7 @@ module Oauth SCOPES = %w[read_prefs write_prefs write_diary write_api read_gpx write_gpx write_notes].freeze PRIVILEGED_SCOPES = %w[read_email skip_authorization].freeze + OAUTH2_SCOPES = %w[openid].freeze class Scope attr_reader :name @@ -14,9 +15,10 @@ module Oauth end end - def self.scopes(privileged: false) + def self.scopes(oauth2: false, privileged: false) scopes = SCOPES scopes += PRIVILEGED_SCOPES if privileged + scopes += OAUTH2_SCOPES if oauth2 scopes.collect { |s| Scope.new(s) } end end From 1f62a2b3429c2cb9376258b7ae1392ce9a9e660b Mon Sep 17 00:00:00 2001 From: Milan Cvetkovic Date: Fri, 15 Sep 2023 12:24:35 +0000 Subject: [PATCH 4/6] Add a test for openid connect support --- Gemfile | 1 + Gemfile.lock | 1 + config/settings/test.yml | 30 +++++++++++++ test/integration/oauth2_test.rb | 74 ++++++++++++++++++++++++++++----- 4 files changed, 96 insertions(+), 10 deletions(-) diff --git a/Gemfile b/Gemfile index bf65965d4..b738fa159 100644 --- a/Gemfile +++ b/Gemfile @@ -149,6 +149,7 @@ group :test do gem "capybara", ">= 2.15" gem "erb_lint", :require => false gem "factory_bot_rails" + gem "jwt" gem "minitest", "~> 5.1" gem "puma", "~> 5.6" gem "rails-controller-testing" diff --git a/Gemfile.lock b/Gemfile.lock index 2a51a3d36..35d67d327 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -586,6 +586,7 @@ DEPENDENCIES jbuilder (~> 2.7) jquery-rails json + jwt kgio kramdown libxml-ruby (>= 2.0.5) diff --git a/config/settings/test.yml b/config/settings/test.yml index 5f0025925..0cfa74cd7 100644 --- a/config/settings/test.yml +++ b/config/settings/test.yml @@ -22,3 +22,33 @@ trace_icon_storage: "test" # Lower some rate limits for testing max_changeset_comments_per_hour: 30 moderator_changeset_comments_per_hour: 60 +# Private key for signing id_tokens +doorkeeper_signing_key: | + -----BEGIN PRIVATE KEY----- + MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDA4pSRHIicerJQ + BvIy9lGJ6ZQA7SAGVM8QeNMBaQftS+ROMY+6CFCJ0kiwb9oDdtFNyo3gpgmlULMC + q0C96r0UllKnTSkHSntkKM0wy3TX0pa8QBaJbbcOXU13xu5cR7ffvtn2kQX8RZc9 + eZtE/bSybNBDSiS4mbP31cSQ71EYsbfD3UiWOpOTbc6Xlw9kCkUjwXk36Jnim7gF + 1kFjD3Vq94ij4OVNxcFp+btrfhq2tsiXa9IPBlt1xTetHwj5HrxseDu2qNQNgPxY + ivFAA3t4BuIuou1HjAdzfqp7Ylsr2b7qx+w+Y9TqhH6AcKd0i1qxh6bYnJezH9JU + BjlJvJMhAgMBAAECggEAAID2/gldiqRqunkc1n48iJ2CufKPRAT3r3rT+OvNzf8F + 6csJAuWKVE8ndR0trBb6L/eooYloJWA4aiLes0BIMyQQs4go5HB7hwTw7ZYycsKF + i0NS676iHO2odKN2iZN/CvIO1AKH9KM35GdgvPA5XG1FU/pUbeOqNn+pQ5mkaWnt + kt+ndBpJQDPSS7nTY8g2BCh97SJSbxEPAccAqNLSvKQED4QVygC63jYZNPDxkJWI + guzNn4wv1AfM0DU4W5fI0UtNSxcWSsefWBJTOKO/uQr/XJglxVh6uKof1dnBZiJD + KU6/+bR1cXoKQ05HAcEcf/mtjJGwnze41p1EI22gYQKBgQDB+VZJwvxlME1MgEGJ + WFPPKiQspKjS0kgbfBw7Iny+mYM6YLpQyF0NFNRloALW2rHH2QLNSerHMlytZUAd + 1SluQZ4We6P3hLDi2J3p37lkIdBXhjJi8gdoEfQ1YVcCbPGbR2ZVwYms7BP3yiQY + ZLcHLUKPKG6hOZztY1gBYqoKqQKBgQD+kBtR8krdJHPEU3m+d/6NWlGk4KZgCFx5 + ouN/aHtxE6Ge+mUwbrJun/oVrFjbX7ySYTYYb6SdKUrchyKfJL4Z89WHGwrFTV1/ + 6J2ShXmoeUeic1TS4btcnFmZyCXlADk1eyHZm9wtkwd5e2lBfdRxzErKC42lWdaQ + rreP2nZHuQKBgQCiNbVgB6vznrn1kIe9qFylsJMBtkzryCe+vEILfaKd7VhdOEh2 + h6ew6ctYlL/rFoV3H1YFgJvSKp5v7mz4xapY5oyiNpD+yzr06LrdulaZkuFcX//A + 2K8y61iyTw1pHNvKw6Gjcy6DqgRkwej/cTHR0ZqIhwJE1x4RMnOE7RJPyQKBgQCM + SLYFjtSa0b/KbYYl5NKu6xsbFYIaYgE0NwPP7rA4PG1QwwSIkDhcpmSXFQdSvYuZ + z2CUTtIUmfDbXs1BjmoEu07syYZB/MSN/I75c/z7TvqfF5ejLyqlerQV/yqC7ICa + bGTXGwFXTDNOSyhSIxm0LLT6ayt/9+Y6jU4zRFzyYQKBgGiScevkv/XNz9MXswJ+ + 2bEIJNIJn0wIeuopifcDQrOTeCK+037t1AQ3lxMXisJABwG1jfw7WTjF3zz4dSUX + cK1+/2V+OkM/0nXjxPwPj7LiOediUyZNUn48r29uGOL1S83PSUdyST207CP6mZjc + K8aJmnGsVEAcWPzbpNh14q/c + -----END PRIVATE KEY----- diff --git a/test/integration/oauth2_test.rb b/test/integration/oauth2_test.rb index 81f12f7cb..b7c6f3a34 100644 --- a/test/integration/oauth2_test.rb +++ b/test/integration/oauth2_test.rb @@ -1,4 +1,5 @@ require "test_helper" +require "jwt" class OAuth2Test < ActionDispatch::IntegrationTest def test_oauth2 @@ -12,7 +13,8 @@ class OAuth2Test < ActionDispatch::IntegrationTest token = request_token(client, code) - test_token(token, user, client) + assert_equal "read_prefs", token["scope"] + test_token(token["access_token"], user, client) end def test_oauth2_oob @@ -30,7 +32,8 @@ class OAuth2Test < ActionDispatch::IntegrationTest token = request_token(client, code) - test_token(token, user, client) + assert_equal "read_prefs", token["scope"] + test_token(token["access_token"], user, client) end def test_oauth2_pkce_plain @@ -46,7 +49,8 @@ class OAuth2Test < ActionDispatch::IntegrationTest token = request_token(client, code, verifier) - test_token(token, user, client) + assert_equal "read_prefs", token["scope"] + test_token(token["access_token"], user, client) end def test_oauth2_pkce_s256 @@ -62,16 +66,67 @@ class OAuth2Test < ActionDispatch::IntegrationTest token = request_token(client, code, verifier) - test_token(token, user, client) + assert_equal "read_prefs", token["scope"] + test_token(token["access_token"], user, client) + end + + def test_openid_connect + user = create(:user) + client = create(:oauth_application, :redirect_uri => "https://some.web.app.example.org/callback", :scopes => "openid read_prefs") + state = SecureRandom.urlsafe_base64(16) + verifier = SecureRandom.urlsafe_base64(48) + challenge = Base64.urlsafe_encode64(Digest::SHA256.digest(verifier), :padding => false) + + authorize_client(user, client, :state => state, :code_challenge => challenge, :code_challenge_method => "S256", :scope => "openid read_prefs") + assert_response :redirect + code = validate_redirect(client, state) + + token = request_token(client, code, verifier) + + assert_equal "openid read_prefs", token["scope"] + + access_token = token["access_token"] + assert_not_nil access_token + + id_token = token["id_token"] + assert_not_nil id_token + + data, _headers = JWT.decode id_token, Doorkeeper::OpenidConnect.signing_key.keypair, true, { + :algorithm => [Doorkeeper::OpenidConnect.signing_algorithm.to_s], + :verify_iss => true, + :iss => "#{Settings.server_protocol}://#{Settings.server_url}", + :verify_sub => true, + :sub => user.id, + :verify_aud => true, + :aud => client.uid + } + + assert_equal user.id.to_s, data["sub"] + assert_not data.key?("preferred_username") + + get oauth_userinfo_path + assert_response :unauthorized + + auth_header = bearer_authorization_header(access_token) + get oauth_userinfo_path, :headers => auth_header + assert_response :success + + userinfo = response.parsed_body + + assert_not_nil userinfo + assert_equal user.id.to_s, userinfo["sub"] + assert_equal user.display_name, userinfo["preferred_username"] end private def authorize_client(user, client, options = {}) - options = options.merge(:client_id => client.uid, - :redirect_uri => client.redirect_uri, - :response_type => "code", - :scope => "read_prefs") + options = { + :client_id => client.uid, + :redirect_uri => client.redirect_uri, + :response_type => "code", + :scope => "read_prefs" + }.merge(options) get oauth_authorization_path(options) assert_response :redirect @@ -135,9 +190,8 @@ class OAuth2Test < ActionDispatch::IntegrationTest assert_response :success token = response.parsed_body assert_equal "Bearer", token["token_type"] - assert_equal "read_prefs", token["scope"] - token["access_token"] + token end def test_token(token, user, client) From 2d5acd199b059ec5d508ca967080f128dcd712f3 Mon Sep 17 00:00:00 2001 From: Milan Cvetkovic Date: Fri, 15 Sep 2023 18:03:16 +0000 Subject: [PATCH 5/6] Add test for openid discovery --- test/integration/oauth2_test.rb | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/test/integration/oauth2_test.rb b/test/integration/oauth2_test.rb index b7c6f3a34..35893e4ab 100644 --- a/test/integration/oauth2_test.rb +++ b/test/integration/oauth2_test.rb @@ -91,7 +91,7 @@ class OAuth2Test < ActionDispatch::IntegrationTest id_token = token["id_token"] assert_not_nil id_token - data, _headers = JWT.decode id_token, Doorkeeper::OpenidConnect.signing_key.keypair, true, { + data, _headers = JWT.decode id_token, Doorkeeper::OpenidConnect.signing_key.public_key, true, { :algorithm => [Doorkeeper::OpenidConnect.signing_algorithm.to_s], :verify_iss => true, :iss => "#{Settings.server_protocol}://#{Settings.server_url}", @@ -118,6 +118,28 @@ class OAuth2Test < ActionDispatch::IntegrationTest assert_equal user.display_name, userinfo["preferred_username"] end + def test_openid_discovery + get oauth_discovery_provider_path + assert_response :success + openid_config = response.parsed_body + + assert_equal "#{Settings.server_protocol}://#{Settings.server_url}", openid_config["issuer"] + + assert_equal oauth_authorization_path, URI(openid_config["authorization_endpoint"]).path + assert_equal oauth_token_path, URI(openid_config["token_endpoint"]).path + assert_equal oauth_userinfo_path, URI(openid_config["userinfo_endpoint"]).path + assert_equal oauth_discovery_keys_path, URI(openid_config["jwks_uri"]).path + end + + def test_openid_key + get oauth_discovery_keys_path + assert_response :success + key_info = response.parsed_body + assert key_info.key?("keys") + assert_equal 1, key_info["keys"].size + assert_equal Doorkeeper::OpenidConnect.signing_key.kid, key_info["keys"][0]["kid"] + end + private def authorize_client(user, client, options = {}) From 408f2918d0d4ebebbb651363a3aa95a0e0eb5a82 Mon Sep 17 00:00:00 2001 From: Milan Cvetkovic Date: Thu, 21 Sep 2023 10:51:30 +0000 Subject: [PATCH 6/6] Load openid signing key from endpoint during openid connect test --- test/integration/oauth2_test.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/integration/oauth2_test.rb b/test/integration/oauth2_test.rb index 35893e4ab..fd6b42fec 100644 --- a/test/integration/oauth2_test.rb +++ b/test/integration/oauth2_test.rb @@ -91,7 +91,7 @@ class OAuth2Test < ActionDispatch::IntegrationTest id_token = token["id_token"] assert_not_nil id_token - data, _headers = JWT.decode id_token, Doorkeeper::OpenidConnect.signing_key.public_key, true, { + data, _headers = JWT.decode id_token, nil, true, { :algorithm => [Doorkeeper::OpenidConnect.signing_algorithm.to_s], :verify_iss => true, :iss => "#{Settings.server_protocol}://#{Settings.server_url}", @@ -99,7 +99,13 @@ class OAuth2Test < ActionDispatch::IntegrationTest :sub => user.id, :verify_aud => true, :aud => client.uid - } + } do |headers, _payload| + kid = headers["kid"] + get oauth_discovery_keys_path + keys = response.parsed_body["keys"] + jwk = keys&.detect { |e| e["kid"] == kid } + jwk && JWT::JWK::RSA.import(jwk).public_key + end assert_equal user.id.to_s, data["sub"] assert_not data.key?("preferred_username")