Consolidate auth callbacks and fix Slack controller issues

Replaces #66, #67, #68, #70 with a single cohesive fix:

- Merge check_session_expiry + check_user_token into one authenticate!
  callback, eliminating the double-render risk by design
- SessionsController and Slack::ApplicationController now each skip a
  single authenticate! instead of managing multiple callbacks
- Rename valid_slack_request? to verify_slack_request! and make it halt
  with head :unauthorized when signature verification fails
- Replace head :unprocessable_entity in send_message with logging and
  Sentry.capture_exception so notification failures never affect the
  HTTP response back to Slack

Author: JuanVqz

app/controllers/application_controller.rb

@@ -1,23 +1,16 @@
 class ApplicationController < ActionController::Base
   # Only allow modern browsers supporting webp images, web push, badges, import maps, CSS nesting, and CSS :has.
   allow_browser versions: :modern
-  before_action :check_session_expiry
-  before_action :check_user_token
+  before_action :authenticate!
 
   private
 
-  def check_user_token
-    unless session[:user_token]
-      render "puzzles/login"
-    end
-  end
-
-  def check_session_expiry
+  def authenticate!
     if session[:expires_at].present? && Time.current > session[:expires_at]
       reset_session
-      render "puzzles/login"
-    else
-      session[:expires_at] = 1.hour.from_now
+      render "puzzles/login" and return
     end
+    session[:expires_at] = 1.hour.from_now
+    render "puzzles/login" unless session[:user_token]
   end
 end

app/controllers/sessions_controller.rb

@@ -1,5 +1,5 @@
 class SessionsController < ApplicationController
-  skip_before_action :check_user_token
+  skip_before_action :authenticate!
 
   def create
     auth = request.env["omniauth.auth"]

app/controllers/slack/application_controller.rb

@@ -1,13 +1,13 @@
 class Slack::ApplicationController < ApplicationController
   skip_before_action :verify_authenticity_token
-  skip_before_action :check_user_token
+  skip_before_action :authenticate!
 
-  before_action :valid_slack_request?
+  before_action :verify_slack_request!
 
   private
 
-  def valid_slack_request?
-    @verified ||= verify_slack_signature
+  def verify_slack_request!
+    head :unauthorized unless verify_slack_signature
   end
 
   def verify_slack_signature
@@ -43,7 +43,8 @@ def open_view(view, trigger_id:)
 
   def send_message(message, channel_id:)
     SlackClient::Client.instance.chat_postMessage(channel: channel_id, blocks: message)
-  rescue Slack::Web::Api::Errors::SlackError
-    head :unprocessable_entity
+  rescue Slack::Web::Api::Errors::SlackError => e
+    Rails.logger.error "Failed to send Slack message: #{e.message}"
+    Sentry.capture_exception(e)
   end
 end

test/controllers/puzzles_controller_test.rb

@@ -7,6 +7,22 @@ class PuzzlesControllerTest < ActionDispatch::IntegrationTest
     assert_response :success
   end
 
+  test "unauthenticated request renders login" do
+    get puzzles_path
+    assert_response :success
+    assert_dom "p", "Log in to access the Ruby or Rails admin panel."
+  end
+
+  test "expired session renders login and requires re-authentication" do
+    sign_in
+    get puzzles_path # authenticate! sets session[:expires_at]
+    travel_to 2.hours.from_now do
+      get puzzles_path
+      assert_response :success
+      assert_dom "p", "Log in to access the Ruby or Rails admin panel."
+    end
+  end
+
   test "should show error message when editing puzzle with invalid data" do
     puzzle = puzzles(:one)
 

test/controllers/sessions_controller_test.rb

@@ -1,7 +1,12 @@
 require "test_helper"
 
 class SessionsControllerTest < ActionDispatch::IntegrationTest
-  # test "the truth" do
-  #   assert true
-  # end
+  test "completes OAuth even when session has expired" do
+    sign_in
+
+    travel_to 2.hours.from_now do
+      sign_in
+      assert_redirected_to root_path
+    end
+  end
 end

test/controllers/slack/puzzles_controller_test.rb

@@ -0,0 +1,68 @@
+require "test_helper"
+
+class Slack::PuzzlesControllerTest < ActionDispatch::IntegrationTest
+  setup do
+    ENV["SLACK_SIGNING_SECRET"] = "test_signing_secret"
+  end
+
+  test "rejects request with invalid Slack signature" do
+    post slack_puzzle_path, params: { payload: puzzle_payload },
+      headers: slack_headers(secret: "wrong_secret")
+
+    assert_response :unauthorized
+  end
+
+  test "rejects request with expired Slack timestamp" do
+    post slack_puzzle_path, params: { payload: puzzle_payload },
+      headers: slack_headers(timestamp: Time.now.to_i - 400)
+
+    assert_response :unauthorized
+  end
+
+  test "renders ok when puzzle fails validation" do
+    params = { payload: puzzle_payload(question: "") }
+
+    post slack_puzzle_path, params: params,
+      headers: slack_headers(body: params.to_query)
+
+    assert_response :ok
+  end
+
+  test "renders ok even when Slack notification fails after puzzle is saved" do
+    original = Slack::ApplicationController.instance_method(:send_message)
+    Slack::ApplicationController.define_method(:send_message) { |*| nil }
+
+    params = { payload: puzzle_payload(question: "What is unique about Ruby's blocks?") }
+
+    post slack_puzzle_path, params: params,
+      headers: slack_headers(body: params.to_query)
+
+    assert_response :ok
+  ensure
+    Slack::ApplicationController.define_method(:send_message, original)
+  end
+
+  private
+
+  def puzzle_payload(question: "What is Ruby?")
+    {
+      user: { id: "U123" },
+      view: {
+        state: {
+          values: {
+            question: { question: { value: question } },
+            answer: { answer: { selected_option: { value: "ruby" } } },
+            explanation: { explanation: { value: "It is a programming language." } },
+            link: { link: { value: nil } }
+          }
+        }
+      }
+    }.to_json
+  end
+
+  def slack_headers(secret: ENV["SLACK_SIGNING_SECRET"], timestamp: Time.now.to_i, body: "")
+    ts = timestamp.to_s
+    sig = "v0=" + OpenSSL::HMAC.hexdigest("SHA256", secret, "v0:#{ts}:#{body}")
+    { "X-Slack-Request-Timestamp" => ts, "X-Slack-Signature" => sig }
+  end
+end