Unify issuer block parameters across DiscoveryController and IdToken

[55728]

Closes #238

Problem

The issuer configuration block receives inconsistent parameters depending on the caller:

Caller	Arguments passed
DiscoveryController	issuer.call(request)
IdToken	issuer.call(resource_owner, application)

This means there is no single block signature that works correctly in both contexts. For example, the generator template uses |resource_owner, application|, but when called from DiscoveryController, the request object is passed as resource_owner.

Solution

Introduce Doorkeeper::OpenidConnect.resolve_issuer as a centralized method (following the same pattern as .signing_algorithm and .signing_key) that accepts keyword arguments:

def self.resolve_issuer(resource_owner: nil, application: nil, request: nil)

The new recommended block signature is:

issuer do |resource_owner, application, request|
  request&.base_url || "https://default.example.com"
end

In discovery context, resource_owner and application are nil.
In token context, request is nil.

Backward compatibility

Existing configurations continue to work via arity-based dispatch:

Arity	Signature	Behavior
0	issuer { "..." }	Called with no arguments
1	issuer { |req| ... }	Called with request or resource_owner (legacy)
2	issuer { |owner, app| ... }	Called with resource_owner, application
3+	issuer { |owner, app, req| ... }	Called with all three (new)

Changes

• Add Doorkeeper::OpenidConnect.resolve_issuer with arity-based backward compatibility
• Simplify DiscoveryController#issuer and IdToken#issuer to delegate to resolve_issuer
• Update generator template to use the new 3-argument signature
• Add comprehensive tests for all arity patterns

[Copilot]

Pull request overview

This PR addresses inconsistent arguments passed to the Doorkeeper::OpenidConnect.configuration.issuer callable by centralizing issuer resolution in Doorkeeper::OpenidConnect.resolve_issuer, and updating callers/templates/tests to use the unified behavior across discovery and token issuance.

Changes:

• Added Doorkeeper::OpenidConnect.resolve_issuer(resource_owner:, application:, request:) with arity-based dispatch for backward compatibility.
• Updated DiscoveryController and IdToken to delegate issuer lookup to resolve_issuer.
• Updated generator initializer template and added/updated specs covering issuer block signatures.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
spec/lib/openid_connect_spec.rb	Adds specs for resolve_issuer across static and callable issuer configurations.
spec/lib/id_token_spec.rb	Adds coverage for 3-arity issuer blocks in ID token claims generation.
spec/controllers/discovery_controller_spec.rb	Adds coverage for 3-arity issuer blocks receiving the request in discovery context.
lib/generators/doorkeeper/openid_connect/templates/initializer.rb	Updates generated initializer to recommend a 3-argument issuer block signature.
lib/doorkeeper/openid_connect/id_token.rb	Switches ID token issuer computation to use resolve_issuer.
lib/doorkeeper/openid_connect.rb	Introduces resolve_issuer implementation with arity-based dispatch.
app/controllers/doorkeeper/openid_connect/discovery_controller.rb	Switches discovery issuer computation to use resolve_issuer.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

include('...').twice is not a valid RSpec matcher chain for include and will fail. If you want to assert the substring appears twice, count occurrences (e.g., via scan/count) or use a more direct expectation on the formatted string parts.

Suggested change

	expect(result).to include('RSpec::Mocks::Double').twice
	expect(result.scan('RSpec::Mocks::Double').size).to eq 2

[Copilot]

These arity-1 examples don't actually verify which argument is being passed: both request and resource_owner are RSpec doubles, so class.name is the same and the expectations will pass even if the precedence is wrong. Consider making the block return different values depending on identity (or using distinct concrete classes) so the specs assert request-vs-owner behavior and the “request preferred” rule is truly covered.

Suggested change

	"issuer-#{request_or_owner.class.name}"
	end
	end
	end
	it 'passes request when called from discovery context' do
	result = subject.resolve_issuer(request: request)
	expect(result).to include 'RSpec::Mocks::Double'
	end
	it 'passes resource_owner when called from token context' do
	result = subject.resolve_issuer(resource_owner: resource_owner)
	expect(result).to include 'RSpec::Mocks::Double'
	end
	it 'prefers request over resource_owner when both are present' do
	result = subject.resolve_issuer(resource_owner: resource_owner, request: request)
	expect(result).to eq subject.resolve_issuer(request: request)
	if request_or_owner == request
	'issuer-request'
	elsif request_or_owner == resource_owner
	'issuer-resource-owner'
	end
	end
	end
	end
	it 'passes request when called from discovery context' do
	result = subject.resolve_issuer(request: request)
	expect(result).to eq 'issuer-request'
	end
	it 'passes resource_owner when called from token context' do
	result = subject.resolve_issuer(resource_owner: resource_owner)
	expect(result).to eq 'issuer-resource-owner'
	end
	it 'prefers request over resource_owner when both are present' do
	result = subject.resolve_issuer(resource_owner: resource_owner, request: request)
	expect(result).to eq 'issuer-request'

[Copilot]

resolve_issuer currently does return issuer.to_s unless issuer.respond_to?(:call), which changes the behavior for a missing/nil issuer from nil to "". In discovery this can result in an issuer key with an empty string (since .compact won’t remove it), producing an invalid discovery document and potentially masking misconfiguration. Consider returning the raw value (no to_s) for non-callables, or raising when issuer is blank/nil.

[Copilot]

Proc#arity can be negative for callables with optional args (and some lambdas), and this case issuer.arity will route all negative arities to the else branch (calling with 3 args). That can raise ArgumentError for lambdas that accept at most 1–2 args, breaking backward compatibility. Consider handling negative arities explicitly (e.g., by using issuer.parameters to determine how many positional args to pass, or by mapping negative arity to its minimum required args).

[55728]

Thanks for the review! Addressed #1 and #2. Regarding #3 (nil issuer) and #4 (negative arity): happy to add guards if you'd like, though in practice the DSL uses proc blocks and issuer is a required config. Let me know!

[55728]

CI failures seem related to recent changes on doorkeeper main (uninitialized constant Doorkeeper::AccessGrant, FrozenError in Rails Engine). This appears unrelated to my changes — would it be worth checking if master CI also fails with the latest doorkeeper main?

[nbulaj]

Master branch looks fine 👀

https://github.com/doorkeeper-gem/doorkeeper-openid_connect/actions/runs/23148097725

[55728]

The failing job uses doorkeeper_master.gemfile (pinned to doorkeeper main at 4f328285fa97). The errors (uninitialized constant Doorkeeper::AccessGrant, FrozenError) all occur during Rails.application.initialize! before any of my code runs. This matrix entry tests compatibility with doorkeeper main, which appears to have a recent breaking change. The other CI jobs (using released doorkeeper versions) should pass fine.

[nbulaj]

Can we rebase with the latest master please? 🙏 Thanks!

[55728]

Rebased onto latest master 😊 Thanks!

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[nbulaj]

Thanks!