Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A few ideas #3

Closed
maxx-coffee opened this issue Jul 9, 2014 · 10 comments
Closed

A few ideas #3

maxx-coffee opened this issue Jul 9, 2014 · 10 comments

Comments

@maxx-coffee
Copy link

Hello,

I have a few questions about this gem. I have been working with it for the past 2 days and so far have really enjoyed the implementation.

I was curious your thoughts on removing
before_action :set_user_by_token
after_action :update_auth_header

To allow users more flexibility when they would like to authenticate users. I don't really need check for this information on my public methods. However I can easily include these in my API authentication methods.

When we are calling validate_token do we need to run update_auth_header ? The only reason I ask is it seems almost like an anti-pattern. If we are check to see if that token is valid and generate a new token we canceled the token we validated. I ran in to an issue earlier where tokens were being invalidated after the validate_token request.

Again, great job on this. It has saved me tons of work.

@lynndylanhurley
Copy link
Owner

Hi @joshualcoffee, thank you for your feedback.

First of all, I would recommend using :update_auth_header. Auth tokens are equivalent to user passwords - they can be used by anyone to gain access to a user's account. Changing the token after each request mitigates this problem.

If you are using angular.js, I've built a module that automatically handles updating the token headers on each request: https://github.com/lynndylanhurley/ng-token-auth

That being said, I understand that dealing with the changing tokens is a huge pain. I will add an option to disable changing the tokens ASAP. I might not be able to get to it tonight - feel free to send me a PR if you have the bandwidth.

@lynndylanhurley
Copy link
Owner

Oh, and you don't need to use the filters on every route. They will only be run on controllers that include the DeviseTokenAuth::Concerns::SetUserByToken concern.

The example in the readme shows a situation where all controllers that inherit from the ApplicationController will use the filters, but you can set this up however you want.

This example is similar to the readme example, but certain actions are excluded from the filters using :skip_before_action and :skip_after_action methods:

# app/controllers/application_controller.rb
class ApplicationController < ActionController::Base
  include DeviseTokenAuth::Concerns::SetUserByToken
end

# app/controllers/test_controller.rb
class TestController < ApplicationController
  # selectively exclude certain actions
  skip_before_action :set_user_by_auth_token, only: [:public_route]
  skip_after_action :update_auth_header, only: [:public_route]

  # :set_user_by_auth_token + :update_auth_header will NOT run for this action
  def public_route
    render json: {
      data: {
        message: "Everyone can access this.",
          user: @user
      }
    }, status: 200    
  end

  # :set_user_by_auth_token + :update_auth_header will run for this action
  def members_only
    if @user
      render json: {
        data: {
          message: "Welcome #{@user.name}",
          user: @user
        }
      }, status: 200
    else
      render json: {
        errors: ["Authorized users only."]
      }, status: 401
    end
  end
end

This example shows how to bypass the filters by only including the concern in private controllers:

# app/controllers/application_controller.rb
class ApplicationController < ActionController::Base
  # notice that we do not include the concern here
end

# app/controllers/public_controller.rb
class PublicController < ApplicationController
  # the concern is not included here either, so each action in this controller
  # will be publicly accessible

    def public_route
    render json: {
      data: {
        message: "Everyone can access this.",
          user: @user
      }
    }, status: 200    
  end
end

# app/controllers/private_controller.rb
class PrivateController < ApplicationController
  include DeviseTokenAuth::Concerns::SetUserByToken

  def members_only
    if @user
      render json: {
        data: {
          message: "Welcome #{@user.name}",
          user: @user
        }
      }, status: 200
    else
      render json: {
        errors: ["Authorized users only."]
      }, status: 401
    end
  end
end

Also, if there are no auth headers present, the filters will just terminate immediately. So including them in public methods won't introduce any additional overhead.

@maxx-coffee
Copy link
Author

Hello again,

Thanks for the explanation regarding the tokens. I am using the angular module with this gem. Thats where I noticed the issue. If you call $auth.validateUser() it calls the validate token endpoint. Which updates the token and then allows you to make another request with a new token. Seems it would take a minor hit on performance to keep updating the table rows. However it may be a good idea to port this to redis if redis is available.

I did narrow down the issue of tokens being invalid to angular caching. I have not researched it enough but it seems if you cache a collection it updates the token in the database but never sends the headers back to update the cookie.

The second issue is the biggest snag I hit with using this gem. I have 2 methods one being public_authentication via a token. The other being private authentication via devise + this gem. The public method will still set a user so :update_auth_header will always be called on public methods. This should raise and exception because @cliend_id will not be defined.

def update_auth_header
    if @user
      # update user's auth token (should happen on each request)
      token                    = SecureRandom.urlsafe_base64(nil, false)
      token_hash               = BCrypt::Password.create(token)
      @user.tokens[@client_id] = {
        token:  token_hash,
        expiry: Time.now + 2.weeks
      }
      @user.save

      # update Authorization response header with new token
      response.headers["Authorization"] = "token=#{token} client=#{@client_id} uid=#{@user.uid}"
    end
  end

I understand there are work around by skipping this action. However just like Devise allows you when to call authenticate_user! it would be nice to have this same functionality. That way the user can include the concern on the parent controller and decide when authorization needs to happen.

@lynndylanhurley
Copy link
Owner

Oh I see - so @user is being defined by your public controller. You're right, I did not plan for that case. Could the issue be solved by changing the condition to if @user and @client_id?

@maxx-coffee
Copy link
Author

It seems that does the trick.

Im curious how you got around the limitations of only being able to request one resource at a time.

Example:

Event.Dates($stateParams.eventId).then((data)->
    $scope.dates = data
  )

 Talent.get($stateParams.talentId).then((data)->
      $scope.talent = data
  )

You should receive a 401 when you request get_talent as the token has not changed yet. Essentially you will submit the same token for both request. Im curious your thoughts on keeping the token "alive" for a short period and then expiring it. Its less secure than refreshing the token every request however it would allow users to build dashboards that could call multiple resources.

@lynndylanhurley
Copy link
Owner

That is another great point that I hadn't considered. I will make this a
priority.
On Jul 10, 2014 5:03 PM, "Joshua Coffee" [email protected] wrote:

It seems that does the trick.

Im curious how you got around the limitations of only being able to
request one resource at a time.

Example:

Event.Dates($stateParams.eventId).then((data)->
$scope.dates = data
)

Talent.get($stateParams.talentId).then((data)->
$scope.talent = data
)

You should receive a 401 when you request get_talent as the token has not
changed yet. Essentially you will submit the same token for both request.
Im curious your thoughts on keeping the token "alive" for a short period
and then expiring it. Its less secure than refreshing the token every
request however it would allow users to build dashboards that could call
multiple resources.


Reply to this email directly or view it on GitHub
#3 (comment)
.

@lynndylanhurley
Copy link
Owner

@joshualcoffee - I'll be attempting to solve this problem on this branch: https://github.com/lynndylanhurley/devise_token_auth/tree/token-persistence

The first thing that I'm going to do is to create an initializer variable for disabling the :update_auth_token filter.

After that, I have several ideas that I'm going to try, including some of the suggestions that you mentioned. I'll keep you informed of the progress.

@lynndylanhurley
Copy link
Owner

@joshualcoffee - I've just made several updates that I hope will address your concerns. To summarize:

  • Multiple simultaneous requests can be made to the API using the same token. So the following example should work (using the same auth headers for both requests):

    Event.Dates($stateParams.eventId).then((data)->
      $scope.dates = data
    )
    
    Talent.get($stateParams.talentId).then((data)->
      $scope.talent = data
    )

    Both of the responses will contain the updated auth headers necessary for the next request.

  • You can disable updating the headers after each request. This is done using a config variable in the new initialization file. More details here.

  • Several new methods have been exposed on the User model for token authentication. More details here.

I'll need to do some additional testing, but please review the latest docs and code and let me know if you find any issues with my approach.

@lynndylanhurley
Copy link
Owner

Oh and I almost forgot - I added mysql to the travis build, and I fixed the issue involving default values on the text columns (I just changed the column type to :string and everything seems to be happy). Please verify that this works for you now.

@lynndylanhurley
Copy link
Owner

I've just merged this all into master, and I've updated the readme with an overview of my approach to batch requests:
https://github.com/lynndylanhurley/ng-token-auth#about-batch-requests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants