lynndylanhurley / lynndylanhurley/devise_token_auth

dta_find_by on wrong model with set_user_by_token

Open
#1,629 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Ruby
Stars
3.6k
Forks
1.1k
PR merge metrics
No merged PRs in 30d

Description

Since the issues #399 and #820 are still seem to be unresolved, I have created a new issue thread.
My case would be more like issue #820
(Please forgive the poor English as I am using a translation tool.)

In my app, There are `Admin` and `User`.
User signin by Devise, Admin signin by DeviseTokenAuth.

If `user_signed_in?` or `current_user` is used in the before_action, `set_user_by_token` method is passed `:user`, and `rc` is assigned User.
However, since the User is dedicated to Devise signin, `DeviseTokenAuth::Concerns::User` does not include it.
Therefore, it does not have a `dta_find_by` method, which causes a NoMethodError in `rc.dta_find_by(uid: uid)`.

https://github.com/lynndylanhurley/devise_token_auth/blob/95b7b91beddc6bff09ffc0d4e3a143e8f73cf606/app/controllers/devise_token_auth/concerns/set_user_by_token.rb#L84

## versions
ruby 3.3.0
Rails 7.1.3.3

Devise 4.9.4
DeviseTokenAuth 1.2.3

## models
```user.rb
class User < ApplicationRecord
# Include default devise modules. Others available are:
# :confirmable, :lockable, :timeoutable, :trackable and :omniauthable
devise :database_authenticatable, :registerable, :validatable
end
```

```admin.rb
class Admin < ActiveRecord::Base
# Include default devise modules. Others available are:
# :confirmable, :lockable, :timeoutable, :trackable and :omniauthable
devise :database_authenticatable, :registerable, :validatable
include DeviseTokenAuth::Concerns::User
end
```

## routes
```routes.rb
Rails.application.routes.draw do
devise_for :users

namespace :api, defaults: {format: 'json'} do
mount_devise_token_auth_for 'Admin', at: 'auth', controllers: {
sessions: 'api/auth/sessions',
}
end
end
```
signin -> /api/auth/sign_in
signout -> /api/auth/sign_out
validate -> /auth/validate_token

## controllers
```appication_controller.rb
class ApplicationController < ActionController::Base
before_action :check_signed

private

def check_signed
if user_signed_in?
puts "User signed in!!!"
end
end
end
```

```api/auth/sessions_controller.rb
class Api::Auth::SessionsController < DeviseTokenAuth::SessionsController
skip_before_action :verify_authenticity_token

protected

def set_user_by_token(mapping = nil)
binding.pry
super(mapping)
end
end
```

# fix suggestion
I am not familiar with security and DeviseTokenAuth specifications.

How about implementing an return before doing `rc.dta_find_by`?
```set_user_by_token.rb
unless rc.included_modules.include?(DeviseTokenAuth::Concerns::User)
return
end
```
Maybe it takes away the means to notice when a developer forgets that he should include `DeviseTokenAuth::Concerns::User`

Alternatively, `resource_class` method in `resource_finder.rb` without `Devise.mappings`, change as in `DeviseTokeunAuth.mappings`
Is it possible to set only the list of models needed from `mount_devise_token_auth_for` ?

I'd like to hear everyone's opinion.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with app/controllers/devise_token_auth/concerns/set_user_by_token.rb at the dta_find_by call, then inspect resource_finder.rb and the related Devise mappings. Reproduce the Admin and User setup from the issue and determine an agreed behavior that avoids the NoMethodError without weakening authentication; done requires a reviewed fix and regression coverage for the mixed mappings.

Written by the indexing model from the issue text.

Assessment

Tech stack
rails, ruby
Domain
api, authentication
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.