Remove no_redirect_on_oauth_token_method FF

why:
- To remove FF cruft
- To simplify the code. We were using errors to redirect on missing
  scopes, but explicitly redirecting for all other oauth param issues,
  which was confusing. We don't do that anymore.

closes INTEROP-7446

flag=none

test-plan:
- The tests cover all of these cases, as there should be no user/API
  changes, just code removal/simplification.

Change-Id: I1a5e1f6e07e517d936748fc41a38dd2e7ee8f49d
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/330832
Reviewed-by: Tucker Mcknight <tmcknight@instructure.com>
QA-Review: Tucker Mcknight <tmcknight@instructure.com>
Product-Review: Ryan Hawkins <ryan.hawkins@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>

app/controllers/oauth2_provider_controller.rb

@@ -19,6 +19,7 @@
 #
 
 class OAuth2ProviderController < ApplicationController
+  rescue_from Canvas::OAuth::RequestError, with: :oauth_error
   protect_from_forgery except: %i[token destroy], with: :exception
   before_action :run_login_hooks, only: %i[token]
   skip_before_action :require_reacceptance_of_terms, only: %i[token destroy]
@@ -34,21 +35,21 @@ class OAuth2ProviderController < ApplicationController
     scopes = (params[:scope] || params[:scopes] || "").split
 
     provider = Canvas::OAuth::Provider.new(params[:client_id], params[:redirect_uri], scopes, params[:purpose])
-    begin
-      raise Canvas::OAuth::RequestError, :invalid_client_id unless provider.has_valid_key?
-      raise Canvas::OAuth::RequestError, :invalid_redirect unless provider.has_valid_redirect?
-    rescue Canvas::OAuth::RequestError => e
-      oauth_error(e)
-      return
-    end
 
-    if provider.key.require_scopes? && !provider.valid_scopes?
-      raise Canvas::OAuth::InvalidScopeError, provider.missing_scopes
-    end
+    raise Canvas::OAuth::RequestError, :invalid_client_id unless provider.has_valid_key?
+    raise Canvas::OAuth::RequestError, :invalid_redirect unless provider.has_valid_redirect?
 
     session[:oauth2] = provider.session_hash
     session[:oauth2][:state] = params[:state] if params.key?(:state)
 
+    if provider.key.require_scopes? && !provider.valid_scopes?
+      return redirect_to Canvas::OAuth::Provider.final_redirect(self,
+                                                                state: params[:state],
+                                                                error: "invalid_scope",
+                                                                error_description: "A requested scope is invalid, unknown, malformed, or exceeds the scope granted by the resource owner. " \
+                                                                                   "The following scopes were requested, but not granted: #{provider.missing_scopes.to_sentence(locale: :en)}")
+    end
+
     unless provider.key.authorized_for_account?(@domain_root_account)
       return redirect_to Canvas::OAuth::Provider.final_redirect(self,
                                                                 state: params[:state],
@@ -97,8 +98,6 @@ class OAuth2ProviderController < ApplicationController
                                           :authentication_provider,
                                           pseudonym_session: :unique_id))
     end
-  rescue Canvas::OAuth::RequestError => e
-    Canvas::OAuth::Provider.is_oob?(params[:redirect_uri]) ? oauth_error(e) : redirect_oauth_error(e)
   end
 
   def confirm
@@ -164,8 +163,6 @@ class OAuth2ProviderController < ApplicationController
     increment_request_cost(Setting.get("oauth_token_additional_request_cost", "200").to_i)
 
     render json: token
-  rescue Canvas::OAuth::RequestError => e
-    Account.site_admin.feature_enabled?(:no_redirect_on_oauth_token_method) ? oauth_error(e) : old_silly_behavior(e)
   end
 
   def destroy
@@ -192,20 +189,6 @@ class OAuth2ProviderController < ApplicationController
     render(exception.to_render_data)
   end
 
-  def redirect_oauth_error(exception)
-    redirect_to exception.redirect_uri(params[:redirect_uri])
-  end
-
-  # this method should be removed when the no_redirect_on_oauth_token_method flag is removed
-  def old_silly_behavior(exception)
-    if @should_not_redirect || Canvas::OAuth::Provider.is_oob?(params[:redirect_uri]) || params[:redirect_uri].blank?
-      response["WWW-Authenticate"] = "Canvas OAuth 2.0" if exception.http_status == 401
-      render(exception.to_render_data)
-    else
-      redirect_to exception.redirect_uri(params[:redirect_uri])
-    end
-  end
-
   def grant_type
     @grant_type ||= params[:grant_type] || (
         (!params[:grant_type] && params[:code]) ? "authorization_code" : "__UNSUPPORTED_PLACEHOLDER__"

config/feature_flags/interop_release_flags.yml

@@ -66,18 +66,6 @@ api_auth_error_updates:
     the "status" field will not be localized; it will always be given in
     English.
   applies_to: SiteAdmin
-no_redirect_on_oauth_token_method:
-  state: hidden
-  display_name: Updates to OAuth 2.0 flow to stop redirecting in the 'token' authentication
-  description: |-
-    If enabled, the OAuth 2.0 will now correctly return 4xx status codes to the
-    requesting client instead of redirecting them to themselves
-  applies_to: SiteAdmin
-  environments:
-    development:
-      state: on
-    ci:
-      state: on
 variable_substitution_numeric_to_string:
   state: hidden
   display_name: Returns string values instead of numeric values in variable substitutions
@@ -217,7 +205,7 @@ lti_overwrite_user_url_input_select_content_dialog:
   applies_to: RootAccount
   display_name: Overwrite user LTI URL input on select content dialog
   description: |-
-    If enabled, the user's LTI URL input on the select content dialog will be overwritten 
+    If enabled, the user's LTI URL input on the select content dialog will be overwritten
     if the tool responds with a valid LTI URL.
 lti_find_external_tool_prefer_original_client_id:
   state: hidden

spec/controllers/oauth2_provider_controller_spec.rb

@@ -497,7 +497,7 @@ describe OAuth2ProviderController do
         let(:success_token_keys) { %w[access_token refresh_token user expires_in token_type canvas_region] }
       end
 
-      it "renders a 302 if a code is not provided for an authorization_code grant" do
+      it "renders a 400 if a code is not provided for an authorization_code grant" do
         post :token, params: base_params
 
         assert_status(400)