Changing headers on base class doesn't change them in subclasses

[mltsy]

Given models like:

ApiModel < ActiveResource::Base
  self.site = "http://api.some-site.com"
end

Project < ApiModel
  belongs_to :cluster
end

Cluster < ApiModel
  has_many :projects
end

I want to be able to set authentication headers per request, to use OAuth for authentication. I thought this was possible (it was working, until I logged in with two different users) by just updating the headers:

before_action do
  ApiModel.headers['Authorization'] = "Bearer #{...request.something...}"
end

But in the case of headers, calling the reader on a subclass sets the headers permanently for that subclass. This, to me, seems unintuitive given the fact that all connection options are inherited from the base class to begin with. It seems that I should be able to change them on the base class, and they should be used in all subclasses where they haven't been explicitly overridden.

Is there a recommended way to update headers on all the objects that inherit from ActiveResource::Base without cycling through and updating every one? Would you be against changing the headers reader to avoid setting _headers on the subclass, and just return the merged headers (superclass.headers.merge(headers_state)) every time so that if a header is changed on the superclass, it will be always be returned when calling .headers on a subclass unless explicitly overridden?

This would be more analogous to how site and proxy are handled, from what I can tell. There is no merging happening in those cases, but the important part is that the attribute is never set in the readers, so if it is changed on the superclass, that change is reflected in the subclasses that haven't overridden a given attribute.

[mltsy]

I just realized why it's (probably) done like it is - because of the writer ([]=). If we just return the result of a merge, then setting values on that ephemeral hash aren't stored anywhere they can be accessed later.

So I guess this would actually require creating a new class for headers, like InheritedHash or something that could override the []= operator so that it would set values on the subclass's copy, while the reader falls back to the superclass's copy. That's not so bad though, for the flexibility it allows!

[jeremy]

Agreed that copy-on-read headers are surprising.

Here's an approach that adds native bearer token support to ActiveResource::Connection itself:

module BearerTokenAuthorization
  def self.prepended(base)
    base.attr_accessor :bearer_token
  end

  def authorization_header(http_method, url)
    if bearer_token
      { 'Authorization' => "Bearer #{bearer_token}" }
    else
      super
    end
  end

  private
    def legitimize_auth_type(auth_type)
      auth_type == :bearer or super
    end
end

ActiveResource::Connection.prepend BearerTokenAuthorization

# To manage auth:
def self.with_bearer_token(token)
  self.connection.auth_type = :bearer
  self.connection.bearer_token = @access_token
  yield
ensure
  self.connection.auth_type = nil
  self.connection.bearer_token = nil
end

[lucasklaassen]

@mltsy what did you end up going with here?

[jeremy]

The commit that adds copy-on-read headers (9f76a4a) clearly intends for subclasses to "inherit" superclass headers and for subclass changes to not affect superclass headers, which is why copying on read instead of write is required. As @mltsy suggests, an inheritance aware hash would be required. Together with thread safety, we'd now have an enormous amount of complexity to accomplish something that should feel simple in both concept and implementation. Inheritable mutable hashes considered harmful. 😊

Recommend pushing authorization responsibility to the connection (code example above) since the headers mechanism is not designed for superclass changes that affect copied-on-read subclass headers.

[mltsy]

And I can't recall the solution I ended up using unfortunately (no longer have access to the project), but that recommendation seems like a reasonable place to start :)

[lucasklaassen]

@jeremy @mltsy thanks guys! PR up: #299

[adrien-k]

This is an old issue but somehow we stumbled upon it today :).
The fix does not look that complex, or I might be missing something with threads safety?
Proposed PR: #359

Also, if not fixed, it would be worth updating the README as the implication could be serious: leaking user-session tokens between requests, as one token may "stick" to a subclass.

You can also set any specific HTTP header using the same way. As mentioned above, headers are thread-safe, so you can set headers dynamically, even in a multi-threaded environment:

ActiveResource::Base.headers['Authorization'] = current_session_api_token

[adrien-k]

Just refreshing this thread as my PR got marked as stale.

This is quite a serious issue, as shown by the following example:

If we consider this simple model:

ApiModel < ActiveResource::Base
  self.site = "http://api.some-site.com"
end

Now the API handles two requests, on the same thread, for two users, with different credentials:

(...)
# The controller sets the header based on current user, Bob
ActiveResource::Base.headers['Authorization'] = "Bob's TOKEN"

(...)
# And then makes a an active resource request, with bob's credentiels
ApiModel.find(1) 

# Later on, a request from Alice, on the same thread, sets the header again
ActiveResource::Base.headers['Authorization'] = "Alice's TOKEN"

# but...the request is still made with Bob's credentials, as ApiMode did read the header and kept a stale copy of it!!!
ApiModel.find(1) 

[bettysteger]

i've got the same problem as @adrien-k !!

@adrien-k do you still have the problem, or do you have a fix?