Support multiple redirect_uris when creating OAuth 2.0 Applications (#29192)

This commit is contained in:
Emelia Smith 2024-05-17 15:46:12 +02:00 committed by GitHub
parent 12472e7f40
commit 2da2a1dae9
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 201 additions and 25 deletions

View file

@ -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

View file

@ -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

View file

@ -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)

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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