Fix regression due to warden session scope usage (#85)

* pass scope/resource_name to warden.session in helpers (needed for referencing non-default scopes);

* scaffold Admin model for dummy app;

* add tests for non-default scopes for Refreshable;

* add neutral base#home action without authentication;

* bypass sign_user_in helper for readability;

* remove authenticate_user! function from dummy application_controller (conflicting with authenticate_admin! callback in admin_posts controller);

* add CHANGELOG entry;

CHANGELOG.md

@@ -2,6 +2,15 @@
 
 ## UNRELEASED
 
+Fix regression due to warden session scope usage
+
+Details:
+- Correct warden session usage for refresh\_credentials hook and helper methods (requires scope to be specified)
+- Add Admin model and AdminPosts controller to dummy app for testing;
+- Add tests to confirm resolution;
+
+## UNRELEASED
+
 Summary: Move refresh\_credentials functionality to dedicated hook (Refreshable);
 
 Details:

lib/devise_otp_authenticatable/controllers/helpers.rb

@@ -46,16 +46,16 @@ def ensure_resource!
       def needs_credentials_refresh?(resource)
         return false unless resource.class.otp_credentials_refresh
 
-        (!warden.session[otp_refresh_property].present? ||
-           (warden.session[otp_refresh_property] < DateTime.now)).tap { |need| otp_set_refresh_return_url if need }
+        (!warden.session(resource_name)[otp_refresh_property].present? ||
+           (warden.session(resource_name)[otp_refresh_property] < DateTime.now)).tap { |need| otp_set_refresh_return_url if need }
       end
 
       #
       # credentials are refreshed
       #
       def otp_refresh_credentials_for(resource)
         return false unless resource.class.otp_credentials_refresh
-        warden.session[otp_refresh_property] = (Time.now + resource.class.otp_credentials_refresh)
+        warden.session(resource_name)[otp_refresh_property] = (Time.now + resource.class.otp_credentials_refresh)
       end
 
       #
@@ -84,11 +84,11 @@ def otp_set_trusted_device_for(resource)
       end
 
       def otp_set_refresh_return_url
-        warden.session[otp_refresh_return_url_property] = request.fullpath
+        warden.session(resource_name)[otp_refresh_return_url_property] = request.fullpath
       end
 
       def otp_fetch_refresh_return_url
-        warden.session.delete(otp_refresh_return_url_property) { :root }
+        warden.session(resource_name).delete(otp_refresh_return_url_property) { :root }
       end
 
       def otp_refresh_return_url_property

lib/devise_otp_authenticatable/hooks/refreshable.rb

@@ -1,5 +1,5 @@
 # After each sign in, update credentials refreshed at time
 Warden::Manager.after_set_user except: :fetch do |record, warden, options|
-  warden.session["credentials_refreshed_at"] = (Time.now + record.class.otp_credentials_refresh)
+  warden.session(options[:scope])["credentials_refreshed_at"] = (Time.now + record.class.otp_credentials_refresh)
 end
 

test/dummy/app/controllers/admin_posts_controller.rb

@@ -0,0 +1,85 @@
+class AdminPostsController < ApplicationController
+  before_action :authenticate_admin!
+
+  # GET /posts
+  # GET /posts.json
+  def index
+    @posts = Post.all
+
+    respond_to do |format|
+      format.html # index.html.erb
+      format.json { render json: @posts }
+    end
+  end
+
+  # GET /posts/1
+  # GET /posts/1.json
+  def show
+    @post = Post.find(params[:id])
+
+    respond_to do |format|
+      format.html # show.html.erb
+      format.json { render json: @post }
+    end
+  end
+
+  # GET /posts/new
+  # GET /posts/new.json
+  def new
+    @post = Post.new
+
+    respond_to do |format|
+      format.html # new.html.erb
+      format.json { render json: @post }
+    end
+  end
+
+  # GET /posts/1/edit
+  def edit
+    @post = Post.find(params[:id])
+  end
+
+  # POST /posts
+  # POST /posts.json
+  def create
+    @post = Post.new(params[:post])
+
+    respond_to do |format|
+      if @post.save
+        format.html { redirect_to @post, notice: "Post was successfully created." }
+        format.json { render json: @post, status: :created, location: @post }
+      else
+        format.html { render action: "new" }
+        format.json { render json: @post.errors, status: :unprocessable_entity }
+      end
+    end
+  end
+
+  # PUT /posts/1
+  # PUT /posts/1.json
+  def update
+    @post = Post.find(params[:id])
+
+    respond_to do |format|
+      if @post.update_attributes(params[:post])
+        format.html { redirect_to @post, notice: "Post was successfully updated." }
+        format.json { head :ok }
+      else
+        format.html { render action: "edit" }
+        format.json { render json: @post.errors, status: :unprocessable_entity }
+      end
+    end
+  end
+
+  # DELETE /posts/1
+  # DELETE /posts/1.json
+  def destroy
+    @post = Post.find(params[:id])
+    @post.destroy
+
+    respond_to do |format|
+      format.html { redirect_to posts_url }
+      format.json { head :ok }
+    end
+  end
+end

test/dummy/app/controllers/application_controller.rb

@@ -1,4 +1,3 @@
 class ApplicationController < ActionController::Base
   protect_from_forgery
-  before_action :authenticate_user!
 end

test/dummy/app/controllers/base_controller.rb

@@ -0,0 +1,6 @@
+class BaseController < ApplicationController
+
+  def home
+  end
+
+end

test/dummy/app/models/admin.rb

@@ -0,0 +1,25 @@
+class Admin < PARENT_MODEL_CLASS
+  if DEVISE_ORM == :mongoid
+    include Mongoid::Document
+
+    ## Database authenticatable
+    field :email, type: String, null: false, default: ""
+    field :encrypted_password, type: String, null: false, default: ""
+
+    ## Recoverable
+    field :reset_password_token, type: String
+    field :reset_password_sent_at, type: Time
+  end
+
+  devise :otp_authenticatable, :database_authenticatable, :registerable,
+    :trackable, :validatable
+
+  # Setup accessible (or protected) attributes for your model
+  # attr_accessible :otp_enabled, :otp_mandatory, :as => :otp_privileged
+  # attr_accessible :email, :password, :password_confirmation, :remember_me
+
+  def self.otp_mandatory
+    true
+  end
+
+end

test/dummy/app/views/admin_posts/_form.html.erb

@@ -0,0 +1,25 @@
+<%= form_for([:admin, @post]) do |f| %>
+  <% if @post.errors.any? %>
+    <div id="error_explanation">
+      <h2><%= pluralize(@post.errors.count, "error") %> prohibited this post from being saved:</h2>
+
+      <ul>
+      <% @post.errors.full_messages.each do |msg| %>
+        <li><%= msg %></li>
+      <% end %>
+      </ul>
+    </div>
+  <% end %>
+
+  <div class="field">
+    <%= f.label :title %><br />
+    <%= f.text_field :title %>
+  </div>
+  <div class="field">
+    <%= f.label :body %><br />
+    <%= f.text_area :body %>
+  </div>
+  <div class="actions">
+    <%= f.submit %>
+  </div>
+<% end %>

test/dummy/app/views/admin_posts/edit.html.erb

@@ -0,0 +1,6 @@
+<h1>Editing post</h1>
+
+<%= render 'form' %>
+
+<%= link_to 'Show', @post %> |
+<%= link_to 'Back', admin_posts_path %>

test/dummy/app/views/admin_posts/index.html.erb

@@ -0,0 +1,25 @@
+<h1>Listing posts</h1>
+
+<table>
+  <tr>
+    <th>Title</th>
+    <th>Body</th>
+    <th></th>
+    <th></th>
+    <th></th>
+  </tr>
+
+<% @posts.each do |post| %>
+  <tr>
+    <td><%= post.title %></td>
+    <td><%= post.body %></td>
+    <td><%= link_to 'Show', post %></td>
+    <td><%= link_to 'Edit', edit_admin_post_path(post) %></td>
+    <td><%= link_to 'Destroy', [:admin, post], confirm: 'Are you sure?', method: :delete %></td>
+  </tr>
+<% end %>
+</table>
+
+<br />
+
+<%= link_to 'New Post', new_admin_post_path %>

test/dummy/app/views/admin_posts/new.html.erb

@@ -0,0 +1,5 @@
+<h1>New post</h1>
+
+<%= render 'form' %>
+
+<%= link_to 'Back', admin_posts_path %>

test/dummy/app/views/admin_posts/show.html.erb

@@ -0,0 +1,15 @@
+<p id="notice"><%= notice %></p>
+
+<p>
+  <b>Title:</b>
+  <%= @post.title %>
+</p>
+
+<p>
+  <b>Body:</b>
+  <%= @post.body %>
+</p>
+
+
+<%= link_to 'Edit', edit_admin_post_path(@post) %> |
+<%= link_to 'Back', admin_posts_path %>

test/dummy/app/views/base/home.html.erb

@@ -0,0 +1 @@
+<h1>Hello world!</h1>

test/dummy/config/routes.rb

@@ -1,6 +1,9 @@
 Dummy::Application.routes.draw do
   devise_for :users
+  devise_for :admins
 
   resources :posts
-  root to: "posts#index"
+  resources :admin_posts
+
+  root to: "base#home"
 end

test/dummy/db/migrate/20240604000001_create_admins.rb

@@ -0,0 +1,9 @@
+class CreateAdmins < ActiveRecord::Migration[7.1]
+  def change
+    create_table :admins do |t|
+      t.string :name
+
+      t.timestamps
+    end
+  end
+end

test/dummy/db/migrate/20240604000002_add_devise_to_admins.rb

@@ -0,0 +1,52 @@
+class AddDeviseToAdmins < ActiveRecord::Migration[5.0]
+  def self.up
+    change_table(:admins) do |t|
+      ## Database authenticatable
+      t.string :email, null: false, default: ""
+      t.string :encrypted_password, null: false, default: ""
+
+      ## Recoverable
+      t.string :reset_password_token
+      t.datetime :reset_password_sent_at
+
+      ## Rememberable
+      t.datetime :remember_created_at
+
+      ## Trackable
+      t.integer :sign_in_count, default: 0
+      t.datetime :current_sign_in_at
+      t.datetime :last_sign_in_at
+      t.string :current_sign_in_ip
+      t.string :last_sign_in_ip
+
+      ## Confirmable
+      # t.string   :confirmation_token
+      # t.datetime :confirmed_at
+      # t.datetime :confirmation_sent_at
+      # t.string   :unconfirmed_email # Only if using reconfirmable
+
+      ## Lockable
+      t.integer :failed_attempts, default: 0 # Only if lock strategy is :failed_attempts
+      t.string :unlock_token # Only if unlock strategy is :email or :both
+      t.datetime :locked_at
+
+      ## Token authenticatable
+      t.string :authentication_token
+
+      # Uncomment below if timestamps were not included in your original model.
+      # t.timestamps
+    end
+
+    add_index :admins, :email, unique: true
+    add_index :admins, :reset_password_token, unique: true
+    # add_index :admins, :confirmation_token,   :unique => true
+    add_index :admins, :unlock_token, unique: true
+    add_index :admins, :authentication_token, unique: true
+  end
+
+  def self.down
+    # By default, we don't want to make any assumption about how to roll back a migration when your
+    # model already existed. Please edit below which fields you would like to remove in this migration.
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

test/dummy/db/migrate/20240604000003_devise_otp_add_to_admins.rb

@@ -0,0 +1,28 @@
+class DeviseOtpAddToAdmins < ActiveRecord::Migration[5.0]
+  def self.up
+    change_table :admins do |t|
+      t.string :otp_auth_secret
+      t.string :otp_recovery_secret
+      t.boolean :otp_enabled, default: false, null: false
+      t.boolean :otp_mandatory, default: false, null: false
+      t.datetime :otp_enabled_on
+      t.integer :otp_time_drift, default: 0, null: false
+      t.integer :otp_failed_attempts, default: 0, null: false
+      t.integer :otp_recovery_counter, default: 0, null: false
+      t.string :otp_persistence_seed
+
+      t.string :otp_session_challenge
+      t.datetime :otp_challenge_expires
+    end
+
+    add_index :admins, :otp_session_challenge, unique: true
+    add_index :admins, :otp_challenge_expires
+  end
+
+  def self.down
+    change_table :admins do |t|
+      t.remove :otp_auth_secret, :otp_recovery_secret, :otp_enabled, :otp_mandatory, :otp_enabled_on, :otp_session_challenge,
+        :otp_challenge_expires, :otp_time_drift, :otp_failed_attempts, :otp_recovery_counter, :otp_persistence_seed
+    end
+  end
+end