Merge pull request #1702 from lurz/form-post-error-redirect

Fix bugs for error response in the form_post and error view

CHANGELOG.md

@@ -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
 

app/controllers/doorkeeper/authorizations_controller.rb

@@ -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

app/views/doorkeeper/authorizations/error.html.erb

@@ -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>

app/views/doorkeeper/authorizations/form_post.html.erb

@@ -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 %>

spec/controllers/authorizations_controller_spec.rb

@@ -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
 
@@ -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
@@ -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",