diff --git a/lib/jwt_sessions/session.rb b/lib/jwt_sessions/session.rb index c38a903..f3917c8 100644 --- a/lib/jwt_sessions/session.rb +++ b/lib/jwt_sessions/session.rb @@ -221,7 +221,12 @@ def issue_tokens_after_refresh end def update_refresh_token - @_refresh.update(@_access.uid, @_access.expiration, @_csrf.encoded) + updated = @_refresh.update(@_access.uid, @_access.expiration, @_csrf.encoded) + if updated == false + AccessToken.destroy(@_access.uid, store) + raise Errors::Unauthorized, "Refresh token not found" + end + @refresh_token = @_refresh.token link_access_to_refresh end diff --git a/lib/jwt_sessions/store_adapters/abstract_store_adapter.rb b/lib/jwt_sessions/store_adapters/abstract_store_adapter.rb index 190cb1b..c5215d6 100644 --- a/lib/jwt_sessions/store_adapters/abstract_store_adapter.rb +++ b/lib/jwt_sessions/store_adapters/abstract_store_adapter.rb @@ -20,6 +20,8 @@ def persist_refresh(_uid:, _access_expiration:, _access_uid:, _csrf:, _expiratio raise NotImplementedError end + # Return false when the refresh token no longer exists. A refresh then + # fails instead of writing back a token that a flush has just removed. def update_refresh(_uid:, _access_expiration:, _access_uid:, _csrf:, _namespace:) raise NotImplementedError end diff --git a/lib/jwt_sessions/store_adapters/memory_store_adapter.rb b/lib/jwt_sessions/store_adapters/memory_store_adapter.rb index 6f68512..7e2b30f 100644 --- a/lib/jwt_sessions/store_adapters/memory_store_adapter.rb +++ b/lib/jwt_sessions/store_adapters/memory_store_adapter.rb @@ -46,6 +46,8 @@ def persist_refresh(uid:, access_expiration:, access_uid:, csrf:, expiration:, n end def update_refresh(uid:, access_expiration:, access_uid:, csrf:, namespace: "") + return false if value_if_not_expired(uid, "refresh", namespace.to_s).empty? + update_refresh_fields( uid, namespace.to_s, @@ -53,6 +55,7 @@ def update_refresh(uid:, access_expiration:, access_uid:, csrf:, namespace: "") access_expiration: access_expiration, access_uid: access_uid ) + true end def all_refresh_tokens(namespace) diff --git a/lib/jwt_sessions/store_adapters/redis_store_adapter.rb b/lib/jwt_sessions/store_adapters/redis_store_adapter.rb index 5cdb276..0e96108 100644 --- a/lib/jwt_sessions/store_adapters/redis_store_adapter.rb +++ b/lib/jwt_sessions/store_adapters/redis_store_adapter.rb @@ -7,6 +7,15 @@ class RedisStoreAdapter < AbstractStoreAdapter REFRESH_KEYS = %i[csrf access_uid access_expiration expiration].freeze DEFAULT_POOL_SIZE = 5 + # Updates a refresh token only while it exists, so that a refresh which + # races a flush cannot write the flushed token back. + UPDATE_REFRESH_SCRIPT = <<~LUA + if redis.call("EXISTS", KEYS[1]) == 0 then + return 0 + end + redis.call("HMSET", KEYS[1], "csrf", ARGV[1], "access_expiration", ARGV[2], "access_uid", ARGV[3]) + return 1 + LUA def initialize(token_prefix: JWTSessions.token_prefix, redis_client: nil, **options) @prefix = token_prefix @@ -51,24 +60,27 @@ def fetch_refresh(uid, namespace, first_match = false) def persist_refresh(uid:, access_expiration:, access_uid:, csrf:, expiration:, namespace: nil) key = full_refresh_key(uid, namespace) - update_refresh( - uid: uid, - access_expiration: access_expiration, - access_uid: access_uid, - csrf: csrf, - namespace: namespace + storage.call("HMSET", + key, + :csrf, csrf, + :access_expiration, access_expiration, + :access_uid, access_uid, + :expiration, expiration ) - storage.call("HSET", key, :expiration, expiration) storage.call("EXPIREAT", key, expiration) end def update_refresh(uid:, access_expiration:, access_uid:, csrf:, namespace: nil) - storage.call("HMSET", + updated = storage.call( + "EVAL", + UPDATE_REFRESH_SCRIPT, + 1, full_refresh_key(uid, namespace), - :csrf, csrf, - :access_expiration, access_expiration, - :access_uid, access_uid + csrf, + access_expiration, + access_uid ) + updated == 1 end def all_refresh_tokens(namespace) diff --git a/test/units/jwt_sessions/store_adapters/test_memory_store_adapter.rb b/test/units/jwt_sessions/store_adapters/test_memory_store_adapter.rb index 79fdef2..dee0d74 100644 --- a/test/units/jwt_sessions/store_adapters/test_memory_store_adapter.rb +++ b/test/units/jwt_sessions/store_adapters/test_memory_store_adapter.rb @@ -60,7 +60,7 @@ def test_update_refresh expiration: expiration, namespace: "" ) - store.update_refresh( + assert store.update_refresh( uid: "uid", access_expiration: expiration, access_uid: "access_uid", @@ -71,6 +71,28 @@ def test_update_refresh assert_equal "csrf2", refresh[:csrf] end + def test_update_refresh_after_destroy + expiration = Time.now.to_i + 3600 + store.persist_refresh( + uid: "uid", + access_expiration: expiration, + access_uid: "access_uid", + csrf: "csrf", + expiration: expiration, + namespace: "" + ) + store.destroy_refresh("uid", "") + + refute store.update_refresh( + uid: "uid", + access_expiration: expiration, + access_uid: "access_uid", + csrf: "csrf2", + namespace: "" + ) + assert_equal({}, store.fetch_refresh("uid", "")) + end + def test_all_refresh_tokens expiration = Time.now.to_i + 3600 store.persist_refresh( diff --git a/test/units/jwt_sessions/store_adapters/test_redis_store_adapter.rb b/test/units/jwt_sessions/store_adapters/test_redis_store_adapter.rb index 757be13..f3963c8 100644 --- a/test/units/jwt_sessions/store_adapters/test_redis_store_adapter.rb +++ b/test/units/jwt_sessions/store_adapters/test_redis_store_adapter.rb @@ -11,6 +11,55 @@ def teardown JWTSessions.remove_instance_variable(:@redis_url) if JWTSessions.instance_variable_defined?(:@redis_url) end + def test_update_refresh + store = JWTSessions::StoreAdapters::RedisStoreAdapter.new + expiration = Time.now.to_i + 3600 + store.persist_refresh( + uid: "uid", + access_expiration: expiration, + access_uid: "access_uid", + csrf: "csrf", + expiration: expiration, + namespace: "update_refresh" + ) + + assert store.update_refresh( + uid: "uid", + access_expiration: expiration, + access_uid: "access_uid", + csrf: "csrf2", + namespace: "update_refresh" + ) + assert_equal "csrf2", store.fetch_refresh("uid", "update_refresh")[:csrf] + ensure + store.destroy_refresh("uid", "update_refresh") + end + + def test_update_refresh_after_destroy + store = JWTSessions::StoreAdapters::RedisStoreAdapter.new + expiration = Time.now.to_i + 3600 + store.persist_refresh( + uid: "uid", + access_expiration: expiration, + access_uid: "access_uid", + csrf: "csrf", + expiration: expiration, + namespace: "update_refresh_after_destroy" + ) + store.destroy_refresh("uid", "update_refresh_after_destroy") + + refute store.update_refresh( + uid: "uid", + access_expiration: expiration, + access_uid: "access_uid", + csrf: "csrf2", + namespace: "update_refresh_after_destroy" + ) + assert_empty store.storage.call("KEYS", "#{store.prefix}_update_refresh_after_destroy_*") + ensure + store.destroy_refresh("uid", "update_refresh_after_destroy") + end + def test_error_on_mixed_redis_options assert_raises ArgumentError do JWTSessions::StoreAdapters::RedisStoreAdapter.new( diff --git a/test/units/jwt_sessions/test_session.rb b/test/units/jwt_sessions/test_session.rb index b60b142..fcdc5d2 100644 --- a/test/units/jwt_sessions/test_session.rb +++ b/test/units/jwt_sessions/test_session.rb @@ -336,6 +336,21 @@ def test_flush_namespaced_access_tokens_after_flush_namespaced assert_equal 0, session.flush_namespaced_access_tokens end + def test_refresh_after_concurrent_flush + refresh_token = tokens[:refresh] + racing_store = JWTSessions.token_store.dup + racing_store.define_singleton_method(:update_refresh) do |**fields| + JWTSessions::Session.new.flush_by_token(refresh_token) + super(**fields) + end + racing_session = JWTSessions::Session.new(payload: payload, store: racing_store) + + assert_raises JWTSessions::Errors::Unauthorized do + racing_session.refresh(refresh_token) + end + assert_empty RedisClient.new.call("KEYS", "jwt_*") + end + def test_flush_all refresh_token = @session.instance_variable_get(:"@_refresh") flushed_count = JWTSessions::Session.flush_all