Merge pull request #1639 from doorkeeper-gem/fix-auth-destroy

Add grant type vlaidation to avoid Internal Server Error for DELETE /oauth/authorize endpoint

Author: nbulaj

CHANGELOG.md

@@ -10,6 +10,7 @@ User-visible changes worth mentioning.
 - [#ID] Add your PR description here.
 - [#1602] Allow custom data to be stored inside access grants/tokens.
 - [#1634] Code refactoring for custom token attributes.
+- [#1639] Add grant type validation to avoid Internal Server Error for DELETE /oauth/authorize endpoint.
 
 # 5.6.4
 

app/controllers/doorkeeper/authorizations_controller.rb

@@ -13,11 +13,19 @@ def new
     end
 
     def create
-      redirect_or_render authorize_response
+      redirect_or_render(authorize_response)
     end
 
     def destroy
-      redirect_or_render authorization.deny
+      redirect_or_render(authorization.deny)
+    rescue Doorkeeper::Errors::InvalidTokenStrategy => e
+      error_response = get_error_response_from_exception(e)
+
+      if Doorkeeper.configuration.api_only
+        render json: error_response.body, status: :bad_request
+      else
+        render :error, locals: { error_response: error_response }
+      end
     end
 
     private
@@ -37,7 +45,7 @@ def render_error
         render json: pre_auth.error_response.body,
                status: :bad_request
       else
-        render :error
+        render :error, locals: { error_response: pre_auth.error_response }
       end
     end
 

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

@@ -3,5 +3,7 @@
 </div>
 
 <main role="main">
-  <pre><%= @pre_auth.error_response.body[:error_description] %></pre>
+  <pre>
+    <%= (respond_to?(:error_response) ? error_response : @pre_auth.error_response).body[:error_description] %>
+  </pre>
 </main>

spec/controllers/authorizations_controller_spec.rb

@@ -25,6 +25,8 @@ def query_params
                      scopes: "default"
   end
 
+  let(:response_json_body) { JSON.parse(response.body) }
+
   before do
     Doorkeeper.configure do
       orm DOORKEEPER_ORM
@@ -111,7 +113,6 @@ def query_params
         post :create, params: { client_id: client.uid, response_type: "token", redirect_uri: client.redirect_uri }
       end
 
-      let(:response_json_body) { JSON.parse(response.body) }
       let(:redirect_uri) { response_json_body["redirect_uri"] }
 
       it "renders success after authorization" do
@@ -154,8 +155,6 @@ def query_params
         }
       end
 
-      let(:response_json_body) { JSON.parse(response.body) }
-
       it "renders success after authorization" do
         expect(response).to be_successful
       end
@@ -200,8 +199,6 @@ def query_params
         }
       end
 
-      let(:response_json_body) { JSON.parse(response.body) }
-
       it "renders 400 error" do
         expect(response.status).to eq 400
       end
@@ -231,8 +228,6 @@ def query_params
         }
       end
 
-      let(:response_json_body) { JSON.parse(response.body) }
-
       it "renders 401 error" do
         expect(response.status).to eq 401
       end
@@ -262,8 +257,6 @@ def query_params
         }
       end
 
-      let(:response_json_body) { JSON.parse(response.body) }
-
       it "renders 401 error" do
         expect(response.status).to eq 401
       end
@@ -355,8 +348,6 @@ def query_params
         }
       end
 
-      let(:response_json_body) { JSON.parse(response.body) }
-
       it "renders 400 error" do
         expect(response.status).to eq 400
       end
@@ -386,8 +377,6 @@ def query_params
         }
       end
 
-      let(:response_json_body) { JSON.parse(response.body) }
-
       it "renders 401 error" do
         expect(response.status).to eq 401
       end
@@ -418,8 +407,6 @@ def query_params
         }
       end
 
-      let(:response_json_body) { JSON.parse(response.body) }
-
       it "renders 401 error" do
         expect(response.status).to eq 401
       end
@@ -451,7 +438,6 @@ def query_params
         }
       end
 
-      let(:response_json_body) { JSON.parse(response.body) }
       let(:redirect_uri) { response_json_body["redirect_uri"] }
 
       it "renders 400 error" do
@@ -492,8 +478,6 @@ def query_params
         }
       end
 
-      let(:response_json_body) { JSON.parse(response.body) }
-
       it "renders 400 error" do
         expect(response.status).to eq 400
       end
@@ -838,8 +822,6 @@ def query_params
         }
       end
 
-      let(:response_json_body) { JSON.parse(response.body) }
-
       it "renders success" do
         expect(response).to be_successful
       end
@@ -928,8 +910,6 @@ def query_params
         get :new, params: { an_invalid: "request" }
       end
 
-      let(:response_json_body) { JSON.parse(response.body) }
-
       it "renders bad request" do
         expect(response).to have_http_status(:bad_request)
       end
@@ -960,8 +940,6 @@ def query_params
         }
       end
 
-      let(:response_json_body) { JSON.parse(response.body) }
-
       it "renders bad request" do
         expect(response).to have_http_status(:bad_request)
       end
@@ -992,8 +970,6 @@ def query_params
         }
       end
 
-      let(:response_json_body) { JSON.parse(response.body) }
-
       it "renders 400 error" do
         expect(response.status).to eq 400
       end
@@ -1106,4 +1082,26 @@ def query_params
       expect(response).to be_successful
     end
   end
+
+  describe "DELETE #destroy in API mode" do
+    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",
+          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 error in body" do
+        expect(response_json_body["error"]).to eq("unsupported_grant_type")
+      end
+    end
+  end
 end