Make error response not redirectable when client is unauthorized

[linhdangduy]

Summary

1. Fix bug: unauthorized_client error on pre_authorization may be returned by redirection to client.

As rfc6749 has mentioned, when client is invalid, it MUST_NOT redirect the user-agent: https://tools.ietf.org/html/rfc6749#section-4.1.2.1

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.

• return status code as 401 when error is unauthorized_client lib/doorkeeper/oauth/error_response.rb

• add name != :unauthorized_client && to redirectable? method of error_response lib/doorkeeper/oauth/error_response.rb

• add unit test on redirectable? for error_response_spec and invalid_request_response_spec spec/lib/oauth/error_response_spec.rb, spec/lib/oauth/invalid_request_response_spec.rb

• add test must call the validations on client and redirect_uri before other validations because they are not redirectable to pre_authorization_spec. spec/lib/oauth/pre_authorization_spec.rb

  • Explain: pre_authorization validations return first error it encounters. With the authorization_request that has two errors: missing parameter response_type and invalid_redirect_uri, if the missing parameter response_type validation is run before invalid_redirect_uri validation, authorization server will return error by redirect the user-agent to the invalid redirect_uri, which is not followed quote from rfc6749 above (which lead to vulnerability on redirection)
  • So I add this test to make sure un-redirectable validations must be run before others.

Other Information

• reordering the validate_resource_owner_authorize_for_client in pre_authorization as order of validate declaration. lib/doorkeeper/oauth/pre_authorization.rb
• delete duplicated rspecs, and unnecessary rspecs on pre_authorization_spec.

[nbulaj]

Looks good so far 👍

[nbulaj]

Let's introduce a const named NON_REDIRECTABLE_STATES or smth like this with states from above and then just:

NON_REDIRECTABLE_STATES.include?(name)

[linhdangduy]

@nbulaj I added these error names to NON_REDIRECTABLE_STATES

[nbulaj]

Thanks @linhdangduy !