From: Tom Hughes Date: Tue, 3 Oct 2023 18:39:41 +0000 (+0100) Subject: Merge remote-tracking branch 'upstream/pull/4226' X-Git-Tag: live~527 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/c8fc2218e5e342ad7afc9409974fd48a733ba94b?hp=370d2b644d359ff435df8c171887abafed753f6c Merge remote-tracking branch 'upstream/pull/4226' --- diff --git a/Gemfile b/Gemfile index ebbebe4f9..724dd3bec 100644 --- a/Gemfile +++ b/Gemfile @@ -81,6 +81,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" @@ -150,6 +151,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 03607e061..c8784ed7c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -179,6 +179,9 @@ GEM railties (>= 5) doorkeeper-i18n (5.2.7) 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) @@ -577,6 +580,7 @@ DEPENDENCIES delayed_job_active_record doorkeeper doorkeeper-i18n + doorkeeper-openid_connect erb_lint factory_bot_rails faraday @@ -592,6 +596,7 @@ DEPENDENCIES jbuilder (~> 2.7) jquery-rails json + jwt kgio kramdown libxml-ruby (>= 2.0.5) 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 new file mode 100644 index 000000000..7f409ecbe --- /dev/null +++ b/config/initializers/doorkeeper_openid_connect.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +Doorkeeper::OpenidConnect.configure do + issuer do |_resource_owner, _application| + "#{Settings.server_protocol}://#{Settings.server_url}" + end + + signing_key Settings.doorkeeper_signing_key + + subject_types_supported [:public] + + resource_owner_from_access_token do |access_token| + User.find_by(:id => access_token.resource_owner_id) + end + + auth_time_from_resource_owner do |resource_owner| + # empty block necessary as a workaround to missing configuration + # when no auth_time claim is provided + end + + subject do |resource_owner, _application| + resource_owner.id + end + + protocol do + Settings.server_protocol.to_sym + end + + claims do + claim :preferred_username, :scope => :openid do |resource_owner, _scopes, _access_token| + resource_owner.display_name + end + + claim :email, :scope => :read_email, :response => [:id_token, :user_info] do |resource_owner, _scopes, _access_token| + resource_owner.email + end + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index da346e8e8..cd7d67947 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -559,10 +559,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 @@ -2530,6 +2551,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 404e7b0a3..43c43a793 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -5,6 +5,8 @@ OpenStreetMap::Application.routes.draw do :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 214f8a284..cffd3bd31 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -154,3 +154,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/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/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'), 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 diff --git a/test/integration/oauth2_test.rb b/test/integration/oauth2_test.rb index 81f12f7cb..fd6b42fec 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,95 @@ 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, nil, 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 + } 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") + + 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 + + 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 = {}) - 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 +218,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)