Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bugs for error response in the form_post and error view #1702

Merged
merged 2 commits into from
Mar 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ User-visible changes worth mentioning.

- [#1696] Add missing `#issued_token` method to `OAuth::TokenResponse`
- [#1697] Allow a TokenResponse body to be customized.
- [#1702] Fix bugs for error response in the form_post and error view

## 5.6.9

Expand Down
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