From 2da2a1dae984f4083d1cc2f55642811c696955e6 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Fri, 17 May 2024 15:46:12 +0200 Subject: [PATCH] Support multiple redirect_uris when creating OAuth 2.0 Applications (#29192) --- .../api/v1/apps/credentials_controller.rb | 2 +- app/controllers/api/v1/apps_controller.rb | 4 +- app/lib/application_extension.rb | 6 + .../rest/application_serializer.rb | 14 +- .../rest/credential_application_serializer.rb | 13 ++ spec/requests/api/v1/apps/credentials_spec.rb | 23 ++- spec/requests/api/v1/apps_spec.rb | 164 +++++++++++++++++- 7 files changed, 201 insertions(+), 25 deletions(-) create mode 100644 app/serializers/rest/credential_application_serializer.rb diff --git a/app/controllers/api/v1/apps/credentials_controller.rb b/app/controllers/api/v1/apps/credentials_controller.rb index 6256bed64c..29ab920383 100644 --- a/app/controllers/api/v1/apps/credentials_controller.rb +++ b/app/controllers/api/v1/apps/credentials_controller.rb @@ -4,6 +4,6 @@ class Api::V1::Apps::CredentialsController < Api::BaseController def show return doorkeeper_render_error unless valid_doorkeeper_token? - render json: doorkeeper_token.application, serializer: REST::ApplicationSerializer, fields: %i(name website vapid_key client_id scopes) + render json: doorkeeper_token.application, serializer: REST::ApplicationSerializer end end diff --git a/app/controllers/api/v1/apps_controller.rb b/app/controllers/api/v1/apps_controller.rb index 97177547a2..50feaf1854 100644 --- a/app/controllers/api/v1/apps_controller.rb +++ b/app/controllers/api/v1/apps_controller.rb @@ -5,7 +5,7 @@ class Api::V1::AppsController < Api::BaseController def create @app = Doorkeeper::Application.create!(application_options) - render json: @app, serializer: REST::ApplicationSerializer + render json: @app, serializer: REST::CredentialApplicationSerializer end private @@ -24,6 +24,6 @@ class Api::V1::AppsController < Api::BaseController end def app_params - params.permit(:client_name, :redirect_uris, :scopes, :website) + params.permit(:client_name, :scopes, :website, :redirect_uris, redirect_uris: []) end end diff --git a/app/lib/application_extension.rb b/app/lib/application_extension.rb index 400c51a023..2fea1057cb 100644 --- a/app/lib/application_extension.rb +++ b/app/lib/application_extension.rb @@ -23,6 +23,12 @@ module ApplicationExtension redirect_uri.lines.first.strip end + def redirect_uris + # Doorkeeper stores the redirect_uri value as a newline delimeted list in + # the database: + redirect_uri.split + end + def push_to_streaming_api # TODO: #28793 Combine into a single topic payload = Oj.dump(event: :kill) diff --git a/app/serializers/rest/application_serializer.rb b/app/serializers/rest/application_serializer.rb index 635508a17c..1a7b9265f1 100644 --- a/app/serializers/rest/application_serializer.rb +++ b/app/serializers/rest/application_serializer.rb @@ -1,24 +1,18 @@ # frozen_string_literal: true class REST::ApplicationSerializer < ActiveModel::Serializer - attributes :id, :name, :website, :scopes, :redirect_uri, - :client_id, :client_secret + attributes :id, :name, :website, :scopes, :redirect_uris # NOTE: Deprecated in 4.3.0, needs to be removed in 5.0.0 attribute :vapid_key + # We should consider this property deprecated for 4.3.0 + attribute :redirect_uri + def id object.id.to_s end - def client_id - object.uid - end - - def client_secret - object.secret - end - def website object.website.presence end diff --git a/app/serializers/rest/credential_application_serializer.rb b/app/serializers/rest/credential_application_serializer.rb new file mode 100644 index 0000000000..bfec7d03e8 --- /dev/null +++ b/app/serializers/rest/credential_application_serializer.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class REST::CredentialApplicationSerializer < REST::ApplicationSerializer + attributes :client_id, :client_secret + + def client_id + object.uid + end + + def client_secret + object.secret + end +end diff --git a/spec/requests/api/v1/apps/credentials_spec.rb b/spec/requests/api/v1/apps/credentials_spec.rb index e1455fe799..6e6970ce53 100644 --- a/spec/requests/api/v1/apps/credentials_spec.rb +++ b/spec/requests/api/v1/apps/credentials_spec.rb @@ -20,14 +20,26 @@ describe 'Credentials' do expect(body_as_json).to match( a_hash_including( + id: token.application.id.to_s, name: token.application.name, website: token.application.website, - vapid_key: Rails.configuration.x.vapid_public_key, scopes: token.application.scopes.map(&:to_s), - client_id: token.application.uid + redirect_uris: token.application.redirect_uris, + # Deprecated properties as of 4.3: + redirect_uri: token.application.redirect_uri.split.first, + vapid_key: Rails.configuration.x.vapid_public_key ) ) end + + it 'does not expose the client_id or client_secret' do + subject + + expect(response).to have_http_status(200) + + expect(body_as_json[:client_id]).to_not be_present + expect(body_as_json[:client_secret]).to_not be_present + end end context 'with a non-read scoped oauth token' do @@ -46,11 +58,14 @@ describe 'Credentials' do expect(body_as_json).to match( a_hash_including( + id: token.application.id.to_s, name: token.application.name, website: token.application.website, - vapid_key: Rails.configuration.x.vapid_public_key, scopes: token.application.scopes.map(&:to_s), - client_id: token.application.uid + redirect_uris: token.application.redirect_uris, + # Deprecated properties as of 4.3: + redirect_uri: token.application.redirect_uri.split.first, + vapid_key: Rails.configuration.x.vapid_public_key ) ) end diff --git a/spec/requests/api/v1/apps_spec.rb b/spec/requests/api/v1/apps_spec.rb index acabbc93f0..1f01bddf3c 100644 --- a/spec/requests/api/v1/apps_spec.rb +++ b/spec/requests/api/v1/apps_spec.rb @@ -9,8 +9,9 @@ RSpec.describe 'Apps' do end let(:client_name) { 'Test app' } - let(:scopes) { nil } - let(:redirect_uris) { 'urn:ietf:wg:oauth:2.0:oob' } + let(:scopes) { 'read write' } + let(:redirect_uri) { 'urn:ietf:wg:oauth:2.0:oob' } + let(:redirect_uris) { [redirect_uri] } let(:website) { nil } let(:params) do @@ -26,13 +27,63 @@ RSpec.describe 'Apps' do it 'creates an OAuth app', :aggregate_failures do subject + expect(response).to have_http_status(200) + + app = Doorkeeper::Application.find_by(name: client_name) + + expect(app).to be_present + expect(app.scopes.to_s).to eq scopes + expect(app.redirect_uris).to eq redirect_uris + + expect(body_as_json).to match( + a_hash_including( + id: app.id.to_s, + client_id: app.uid, + client_secret: app.secret, + name: client_name, + website: website, + scopes: ['read', 'write'], + redirect_uris: redirect_uris, + # Deprecated properties as of 4.3: + redirect_uri: redirect_uri, + vapid_key: Rails.configuration.x.vapid_public_key + ) + ) + end + end + + context 'without scopes being supplied' do + let(:scopes) { nil } + + it 'creates an OAuth App with the default scope' do + subject + expect(response).to have_http_status(200) expect(Doorkeeper::Application.find_by(name: client_name)).to be_present body = body_as_json - expect(body[:client_id]).to be_present - expect(body[:client_secret]).to be_present + expect(body[:scopes]).to eq Doorkeeper.config.default_scopes.to_a + end + end + + # FIXME: This is a bug: https://github.com/mastodon/mastodon/issues/30152 + context 'with scopes as an array' do + let(:scopes) { %w(read write follow) } + + it 'creates an OAuth App with the default scope' do + subject + + expect(response).to have_http_status(200) + + app = Doorkeeper::Application.find_by(name: client_name) + + expect(app).to be_present + expect(app.scopes.to_s).to eq 'read' + + body = body_as_json + + expect(body[:scopes]).to eq ['read'] end end @@ -77,8 +128,8 @@ RSpec.describe 'Apps' do end end - context 'with a too-long redirect_uris' do - let(:redirect_uris) { "https://foo.bar/#{'hoge' * 2_000}" } + context 'with a too-long redirect_uri' do + let(:redirect_uris) { "https://app.example/#{'hoge' * 2_000}" } it 'returns http unprocessable entity' do subject @@ -87,8 +138,80 @@ RSpec.describe 'Apps' do end end - context 'without required params' do - let(:client_name) { '' } + # NOTE: This spec currently tests the same as the "with a too-long redirect_uri test case" + context 'with too many redirect_uris' do + let(:redirect_uris) { (0...500).map { |i| "https://app.example/#{i}/callback" } } + + it 'returns http unprocessable entity' do + subject + + expect(response).to have_http_status(422) + end + end + + context 'with multiple redirect_uris as a string' do + let(:redirect_uris) { "https://redirect1.example/\napp://redirect2.example/" } + + it 'creates an OAuth application with multiple redirect URIs' do + subject + + expect(response).to have_http_status(200) + + app = Doorkeeper::Application.find_by(name: client_name) + + expect(app).to be_present + expect(app.redirect_uri).to eq redirect_uris + expect(app.redirect_uris).to eq redirect_uris.split + + body = body_as_json + + expect(body[:redirect_uri]).to eq redirect_uris + expect(body[:redirect_uris]).to eq redirect_uris.split + end + end + + context 'with multiple redirect_uris as an array' do + let(:redirect_uris) { ['https://redirect1.example/', 'app://redirect2.example/'] } + + it 'creates an OAuth application with multiple redirect URIs' do + subject + + expect(response).to have_http_status(200) + + app = Doorkeeper::Application.find_by(name: client_name) + + expect(app).to be_present + expect(app.redirect_uri).to eq redirect_uris.join "\n" + expect(app.redirect_uris).to eq redirect_uris + + body = body_as_json + + expect(body[:redirect_uri]).to eq redirect_uris.join "\n" + expect(body[:redirect_uris]).to eq redirect_uris + end + end + + context 'with an empty redirect_uris array' do + let(:redirect_uris) { [] } + + it 'returns http unprocessable entity' do + subject + + expect(response).to have_http_status(422) + end + end + + context 'with just a newline as the redirect_uris string' do + let(:redirect_uris) { "\n" } + + it 'returns http unprocessable entity' do + subject + + expect(response).to have_http_status(422) + end + end + + context 'with an empty redirect_uris string' do let(:redirect_uris) { '' } it 'returns http unprocessable entity' do @@ -97,5 +220,30 @@ RSpec.describe 'Apps' do expect(response).to have_http_status(422) end end + + context 'without a required param' do + let(:client_name) { '' } + + it 'returns http unprocessable entity' do + subject + + expect(response).to have_http_status(422) + end + end + + context 'with a website' do + let(:website) { 'https://app.example/' } + + it 'creates an OAuth application with the website specified' do + subject + + expect(response).to have_http_status(200) + + app = Doorkeeper::Application.find_by(name: client_name) + + expect(app).to be_present + expect(app.website).to eq website + end + end end end