Merge pull request 'Fix after-sign-in redirect issues' (#243) from bugfix/after-signin_redirects into master

Reviewed-on: #243
Reviewed-by: Greg <greg@kosmos.org>

app/controllers/application_controller.rb

@@ -39,7 +39,7 @@ class ApplicationController < ActionController::Base
   end
 
   def after_sign_in_path_for(user)
-    session[:user_return_to] || root_path
+    session.delete(:user_return_to) || root_path
   end
 
   def lndhub_authenticate(options={})

app/controllers/devise/passwords_controller.rb

@@ -55,7 +55,12 @@ class Devise::PasswordsController < DeviseController
 
   protected
     def after_resetting_password_path_for(resource)
-      Devise.sign_in_after_reset_password ? after_sign_in_path_for(resource) : new_session_path(resource_name)
+      session.delete(:user_return_to)
+      if Devise.sign_in_after_reset_password
+        root_path
+      else
+        new_session_path(resource_name)
+      end
     end
 
     # The path used after sending reset password instructions

app/controllers/rs/oauth_controller.rb

@@ -1,4 +1,5 @@
 class Rs::OauthController < ApplicationController
+  prepend_before_action :assert_redirect_uri, only: [:new, :create]
   before_action :require_signed_in_with_username, only: :new
   before_action :authenticate_user!, only: :create
 
@@ -16,8 +17,6 @@ class Rs::OauthController < ApplicationController
                         ["In 1 month", 1.month.from_now],
                         ["In 1 day", 1.day.from_now]]
 
-    http_status :bad_request and return unless @redirect_uri.present?
-
     unless current_user == @user
       sign_out :user
 
@@ -64,8 +63,6 @@ class Rs::OauthController < ApplicationController
     state        = params[:state].presence
     expire_at    = params[:expire_at].presence
 
-    http_status :bad_request and return unless redirect_uri.present?
-
     if permissions.empty?
       redirect_to(url_with_state("#{redirect_uri}#error=invalid_scope", state),
                   allow_other_host: true) and return
@@ -97,6 +94,10 @@ class Rs::OauthController < ApplicationController
 
   private
 
+  def assert_redirect_uri
+    http_status :bad_request unless params[:redirect_uri].present?
+  end
+
   def require_signed_in_with_username
     unless user_signed_in?
       session[:user_return_to] = request.url

spec/controllers/rs/oauth_controller_spec.rb

@@ -222,6 +222,19 @@ RSpec.describe Rs::OauthController, type: :controller do
 
         expect(response).to redirect_to(new_user_session_path(cn: user.cn, ou: user.ou))
       end
+
+      context "without a redirect_uri" do
+        it "returns a 400 without storing the return location" do
+          get :new, params: {
+            username: user.cn,
+            scope: "documents,photos",
+            client_id: "https://example.com"
+          }
+
+          expect(response.response_code).to eq(400)
+          expect(session[:user_return_to]).to be_nil
+        end
+      end
     end
 
     describe "root access" do

spec/features/devise/password_reset.rb

@@ -50,5 +50,26 @@ RSpec.describe 'Password reset', type: :feature do
       expect(page).to have_content 'Your password has been changed successfully'
       expect(user.reload.reset_password_token).to be_nil
     end
+
+    scenario "Ignores a stale return location left by an rs/oauth request" do
+      expect(Devise::LDAP::Adapter).to receive(:update_password)
+        .with(user.cn, 'catch me if you can').and_return(true)
+
+      # Simulate an earlier rs/oauth authorization request that stored a
+      # return URL in the session (the original cause of issue #235).
+      visit new_rs_oauth_path(user.cn,
+                              redirect_uri: "https://example.com",
+                              client_id: "https://example.com",
+                              scope: "documents")
+
+      visit edit_user_password_path(reset_password_token: token)
+      fill_in :user_password, with: 'catch me if you can'
+      fill_in :user_password_confirmation, with: 'catch me if you can'
+      click_button 'Change my password'
+
+      expect(page).to have_content 'Your password has been changed successfully'
+      expect(page).to have_current_path(root_path)
+      expect(page).not_to have_content 'Bad request'
+    end
   end
 end

spec/requests/devise/passwords_spec.rb

@@ -0,0 +1,36 @@
+require 'rails_helper'
+
+RSpec.describe "Devise password reset", type: :request do
+  let(:user) { create :user }
+
+  describe "PUT /users/password" do
+    let(:token) { user.send(:set_reset_password_token) }
+
+    before do
+      allow(Devise::LDAP::Adapter).to receive(:update_password).and_return(true)
+    end
+
+    context "with a stale stored return location from an rs/oauth request" do
+      before do
+        get new_rs_oauth_url(user.cn,
+                            redirect_uri: "https://example.com",
+                            client_id: "https://example.com",
+                            scope: "documents")
+        expect(session[:user_return_to]).to be_present
+      end
+
+      it "redirects to the dashboard instead of the stored return URL" do
+        put user_password_path, params: {
+          user: {
+            reset_password_token: token,
+            password: "a brand new password",
+            password_confirmation: "a brand new password"
+          }
+        }
+
+        expect(response).to redirect_to(root_path)
+        expect(session[:user_return_to]).to be_nil
+      end
+    end
+  end
+end