Doorkeeper appears to be missing a way to validate client configuration before redirecting to the authentication page

[ransombriggs]

Steps to reproduce

We are using a DIY implementation but it basically boils down to the following configuration.

Doorkeeper.configure do
  resource_owner_authenticator do
    current_user || redirect_to(sign_in_url)
  end
end

When there are no valid application clients go to the /oauth/authorize endpoint with an invalid client_id.

http://localhost:3000/oauth/authorize?client_id=invalid-client-id

Expected behavior

I was expecting to get an error that the client_id was not configured properly. I double checked the spec and it does not say if it should prevent authorization, it just says that it should not be redirected back to the invalid redirection uri. From a usability standpoint, it feels preferable to prevent this sort of behavior earlier rather than after the redirect.

If the request fails due to a missing, invalid, or mismatching redirection URI, or if the client identifier is missing or invalid, the authorization server SHOULD inform the resource owner of the error and MUST NOT automatically redirect the user-agent to the invalid redirection URI.

I did look at a workaround for this from resource_owner_authenticator itself, but it was really awkward because pre_auth calls this method as well. I think that in order to get this to behave how I would expect, the PreAuthorization would need to be split so that we could validate before we have an identity, then authorize after that the identity is valid for the client.

Actual behavior

I was redirected to sign_in_url and did not get an error until authenticated.

System Information

Gemfile.lock:

Gemfile.lock content

    doorkeeper (5.7.0)
      railties (>= 5)

[ransombriggs]

For what it is worth I was able to hack in this functionality, which is gross because I am relying on doorkeeper internals to do so. I am feeling more comfortable with the doorkeeper code and could turn this into an actual PR if y'all think this would be a good idea.

  resource_owner_authenticator do
    if action_name == "new"
      # NOTE: this exists because doorkeeper only has oauth errors after users log in and we want it to error beforehand
      pre_authorization = Doorkeeper::OAuth::PreAuthorization.new(
        Doorkeeper.configuration,
        pre_auth_params,
        nil # in the pre_auth method this is current_resource_owner but that calls this method which would be a loop
      )

      unless pre_authorization.authorizable?
        # HACK: use pre_authorization in the memoized variable to prevent an infinite loop
        @pre_auth = pre_authorization
        render_error # this uses the memoized @pre_auth internally
        next
      end
    end
    
    current_user || redirect_to(sign_in_url)
  end