Skip to content

Commit

Permalink
Fix bugs for error response in the form_post and error view
Browse files Browse the repository at this point in the history
Bug
When an error is rendered with the `form_post.html.erb` view, an empty
form is posted instead of the correct error and error_description.

Root cause
The form_post view used the body of `@authorize_response`. However,
the `redirect_or_render` could be called with `authorization.deny` or
`pre_auth.error_response`. In this case, `@authorize_response` would
only contain an empty body.

Fix
Instead of using `@authorize_response`, we pass in a local variable
`auth` to the `form_post.html.erb`, which corresponding to the auth
object passed into the redirect_or_render function. Tests are backfilled
to the authorizations controller spec.

During the test phase, I discovered error.html.erb always renders an
incorrect error description. It turns out that `respond_to(:error_respon
se)` always return false in the view. I changed it to use
`local_assigns` as the correct condition.
  • Loading branch information
lurz committed Mar 22, 2024
1 parent e93f2d7 commit a888789
Show file tree
Hide file tree
Showing 4 changed files with 137 additions and 4 deletions.
2 changes: 1 addition & 1 deletion app/controllers/doorkeeper/authorizations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ def redirect_or_render(auth)
)
end
elsif pre_auth.form_post_response?
render :form_post
render :form_post, locals: { auth: auth }
else
redirect_to auth.redirect_uri, allow_other_host: true
end
Expand Down
2 changes: 1 addition & 1 deletion app/views/doorkeeper/authorizations/error.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@

<main role="main">
<pre>
<%= (respond_to?(:error_response) ? error_response : @pre_auth.error_response).body[:error_description] %>
<%= (local_assigns[:error_response] ? error_response : @pre_auth.error_response).body[:error_description] %>
</pre>
</main>
2 changes: 1 addition & 1 deletion app/views/doorkeeper/authorizations/form_post.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
</header>

<%= form_tag @pre_auth.redirect_uri, method: :post, name: :redirect_form, authenticity_token: false do %>
<% @authorize_response&.body&.compact&.each do |key, value| %>
<% auth.body.compact.each do |key, value| %>
<%= hidden_field_tag key, value %>
<% end %>
<% end %>
Expand Down
135 changes: 134 additions & 1 deletion spec/controllers/authorizations_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,10 @@ def query_params
it "does not issue any access token" do
expect(Doorkeeper::AccessToken.all).to be_empty
end

it "includes the error in the redirect post" do
expect(response.body).to include("invalid_scope")
end
end
end

Expand Down Expand Up @@ -1100,6 +1104,28 @@ def query_params
end
end

context "invalid scope with form_post response mode" do
before do
default_scopes_exist :public
get :new, params: {
client_id: client.uid,
response_type: "token",
scope: "invalid",
redirect_uri: client.redirect_uri,
state: "return-this",
response_mode: "form_post",
}
end

it "renders the form_post page" do
expect(response.status).to eq(200)
end

it "includes the error in the redirect post" do
expect(response.body).to include("invalid_scope")
end
end

context "invalid redirect_uri" do
before do
default_scopes_exist :public
Expand Down Expand Up @@ -1296,10 +1322,117 @@ def query_params
end
end

describe "DELETE #destroy" do
context "without form_post response mode" do
before do
delete :destroy, params: {
client_id: client.uid,
response_type: "token",
redirect_uri: client.redirect_uri,
}
end

it "redirects" do
expect(response).to be_redirect
end

it "redirects to client redirect uri" do
expect(response.location).to match(/^#{client.redirect_uri}/)
end

it "includes error in fragment" do
expect(response.query_params["error"]).to eq("access_denied")
end
end

context "with form_post response mode" do
before do
delete :destroy, params: {
client_id: client.uid,
response_type: "token",
redirect_uri: client.redirect_uri,
response_mode: "form_post",
}
end

it "redirects after authorization" do
expect(response.status).to eq(200)
end

it "includes the error in the redirect post" do
expect(response.body).to include("access_denied")
end
end

context "with invalid params" do
before do
delete :destroy, params: {
client_id: client.uid,
response_type: "blabla",
redirect_uri: client.redirect_uri,
}
end

it "renders the error page correctly" do
expect(response.status).to eq(200)
end

it "includes the error in the page" do
expect(response.body).to include(
translated_error_message(:unsupported_grant_type),
)
end
end
end

describe "DELETE #destroy in API mode" do
before do
allow(Doorkeeper.config).to receive(:api_only).and_return(true)
end

context "without form_post response mode" do
before do
delete :destroy, params: {
client_id: client.uid,
response_type: "token",
redirect_uri: client.redirect_uri,
}
end

it "renders bad request" do
expect(response).to have_http_status(:bad_request)
end

it "includes access_denied in the redirect uri" do
expect(response_json_body["redirect_uri"].match(/error=(\w+)&?/)[1]).to eq("access_denied")
end
end

context "with form_post response mode" do
before do
delete :destroy, params: {
client_id: client.uid,
response_type: "token",
redirect_uri: client.redirect_uri,
response_mode: "form_post",
}
end

it "renders bad request" do
expect(response).to have_http_status(:bad_request)
end

it "includes the correct redirect uri" do
expect(response_json_body["redirect_uri"]).to eq(client.redirect_uri)
end

it "includes access_denied in the body" do
expect(response_json_body["body"]["error"]).to eq("access_denied")
end
end

context "with invalid params" do
before do
allow(Doorkeeper.config).to receive(:api_only).and_return(true)
delete :destroy, params: {
client_id: client.uid,
response_type: "blabla",
Expand Down

0 comments on commit a888789

Please sign in to comment.