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

Avoid sending auth headers if while processing used token is cleared #531

Conversation

virginia-rodriguez
Copy link
Contributor

This pull request fix the error occurred in following scenario:

  1. Client sends request R1 and server starts processing it
  2. Client sends sign out request R2 and server process it (R1 still processing)
  3. R1 finishes. Its response to the server will contain auth headers, although user already signed out.

To avoid R1 response containing auth headers, in the after action code, it should be checked that the token had not been removed in the meantime.

@@ -74,6 +74,10 @@ def update_auth_header
@client_id = nil unless @used_auth_by_token

if @used_auth_by_token and not DeviseTokenAuth.change_headers_on_each_request
# should not append auth header if @resource related token was
# cleared by sign out in the meantime
return if @resource.reload.tokens[@client_id].nil?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@virginia-rodriguez - any implementation ideas that don't involve the additional reload call? i'm personally aiming for sub-200ms roundtrip responses in my apps these days and that's pretty hard to do with Rails and additional ActiveRecord calls. =]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@booleanbetrayal I can't think of any way to avoid the reload. In fact, when DeviseTokenAuth.change_headers_on_each_request is true (default value, right?) an implicit reload is already done by using @resource.with_lock

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah ... me neither. was trying to avoid that call when change_headers_on_request is false. Might need to revisit optimistic locking in general. Couple of other related issues. Merging and may revisit later!

@booleanbetrayal
Copy link
Collaborator

Thanks for this PR @virginia-rodriguez . I had one inline comment. I'm wondering if we can avoid some DB overhead. If we can't think of something incredibly creative soon, I'll go ahead and merge it.

booleanbetrayal added a commit that referenced this pull request Feb 15, 2016
…er-action

Avoid sending auth headers if while processing used token is cleared
@booleanbetrayal booleanbetrayal merged commit afe6da1 into lynndylanhurley:master Feb 15, 2016
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

Successfully merging this pull request may close these issues.

2 participants