Skip to content

Commit ae642bb

Browse files
committed
Drop token from authenticator cache
I'm not sure why we originally wanted to add the token to the cache. This cache isn't preventing a network request. It's simply preventing a potential database write. This means the only thing we need to be concerned about for the cache is the contents of the response. By removing the token we no longer have references to it stored in the clear. This sets up the cache key based on the provider (though this is likely static for most apps), the user id, and the response body contents. If the response already has an etag we'll use it, otherwise we generate a simple one based on the JSON representation. We use MD5 because that's the default that Rails uses. This isn't for security, simply cache lookups so we're not really concerned about slowing the hash or collisions.
1 parent 403a13c commit ae642bb

File tree

1 file changed

+21
-6
lines changed

1 file changed

+21
-6
lines changed

lib/kracken/authenticator.rb

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,13 @@ def self.cache
1919
# authentication to the auth server, normally from a mobile app
2020
def self.user_with_credentials(email, password)
2121
auth = Kracken::CredentialAuthenticator.new.fetch(email, password)
22-
self.new(auth.body).to_app_user
22+
new(auth).to_app_user
2323
end
2424

2525
# Login the user with an auth token. Used for API authentication for the
2626
# public APIs
2727
def self.user_with_token(token)
28-
auth = Kracken::TokenAuthenticator.new.fetch(token)
28+
auth = new(Kracken::TokenAuthenticator.new.fetch(token))
2929

3030
# Don't want stale user models being pulled from the cache. So only
3131
# cache the `user_id`.
@@ -34,15 +34,30 @@ def self.user_with_token(token)
3434
# for the user, set it to nil, fetch from cache and only query if there
3535
# was a cache-hit (thus user is still nil).
3636
user = nil
37-
user_id = Authenticator.cache.fetch("#{token}/#{auth.etag}") {
38-
user = self.new(auth.body).to_app_user
37+
user_id = Authenticator.cache.fetch(auth) {
38+
user = auth.to_app_user
3939
user.id
4040
}
4141
user ||= Kracken.config.user_class.find(user_id)
4242
end
4343

4444
def initialize(response)
45-
@auth_hash = create_auth_hash(response)
45+
@auth_hash = create_auth_hash(response.body)
46+
@etag = if response.respond_to?(:etag)
47+
response.etag
48+
else
49+
# https://github.com/rails/rails/blob/v5.2.0/actionpack/lib/action_dispatch/http/cache.rb#L136
50+
# https://github.com/rails/rails/blob/v5.2.0/activesupport/lib/active_support/digest.rb
51+
# https://github.com/rails/rails/blob/v4.2.10/actionpack/lib/action_dispatch/http/cache.rb#L87
52+
::Digest::MD5.hexdigest(response.body.to_json)
53+
end
54+
@etag.freeze
55+
end
56+
57+
attr_reader :etag
58+
59+
def cache_key
60+
"#{@auth_hash.provider || :unknown}/#{@auth_hash.uid}-#{etag}"
4661
end
4762

4863
# Convert this Factory to a User object per the host app.
@@ -51,7 +66,7 @@ def to_app_user
5166
Kracken.config.user_class.find_or_create_from_auth_hash(auth_hash)
5267
end
5368

54-
private
69+
private
5570

5671
def create_auth_hash(response_hash)
5772
Hashie::Mash.new({

0 commit comments

Comments
 (0)