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

Fix require_user! behavior when not logged in #4604

Merged
merged 1 commit into from Aug 15, 2017

Conversation

@abcang
Copy link
Collaborator

commented Aug 15, 2017

If you run require_user! when you are not logged in, set_user_activity raises an exception.

> undefined method `update_tracked_fields!' for nil:NilClass

app/controllers/concerns/user_tracking_concern.rb, line 17
----------------------------------------------------------

``` ruby
   12   
   13     private
   14   
   15     def set_user_activity
   16       # Mark as signed-in today
>  17       current_user.update_tracked_fields!(request)
   18   
   19       # Regenerate feed if needed
   20       regenerate_feed! if user_needs_feed_update?
   21     end
   22   

Therefore, I changed to execute set_user_activity only when current_user is not nil.

@Gargron Gargron merged commit e120d09 into tootsuite:master Aug 15, 2017
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nightpool

This comment has been minimized.

Copy link
Collaborator

commented Aug 15, 2017

Why the behavior change to using current_user over current_resource_owner here?

@abcang abcang deleted the abcang:fix/require_user branch Aug 15, 2017
@abcang

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 15, 2017

In current_resource_owner, if doorkeeper_token is falsy, it returns nil without raising an exception. So, set_user_activity may be executed when you are not logged in. current_user in Api::BaseController is executing current_resource_owner and returns nil if not logged in.

However, I noticed that there is a possibility of executing current_user provided by devise. This may be bad.

@abcang

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 16, 2017

It seems that the behavior to verify users authenticated with doorkeeper or devise is correct. In Api::Web::SettingsController, doorkeeper is not used, but require_user! is executed. So if you change to the following behavior, it will not work with the API controller that does not use doorkeeper.

  def current_resource_owner
    @current_user ||= User.find(doorkeeper_token.resource_owner_id) if doorkeeper_token
  rescue ActiveRecord::RecordNotFound
    nil
  end

  def current_user
    current_resource_owner || super
  end

  def require_user!
    if current_resource_owner
      set_user_activity
    else
      render json: { error: 'This method requires an authenticated user' }, status: 422
    end
  end
yukimochi added a commit to yukimochi/mastodon that referenced this pull request Aug 16, 2017
YaQ00 added a commit to YaQ00/mastodon that referenced this pull request Sep 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.