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

Implement MemoryStore #39

Merged
merged 1 commit into from Nov 8, 2018
Merged

Implement MemoryStore #39

merged 1 commit into from Nov 8, 2018

Conversation

@DmitryTsepelev
Copy link
Contributor

@DmitryTsepelev DmitryTsepelev commented Oct 20, 2018

Here is a very basic implementation of abstract adapter for storages.

The idea is that we keep all the code we had in the Redis adapter and just use the appropriate store object for the each specific implementation. This object should be able to respond to all the methods we're using (get, expire at etc), for the in-memory adapter I've used a mock_redis gem (if we don't want to have this dependency - I can replace it with my own implementation). If we once decide to add for instance a postgres support - we'd just need to write the class implementing all the methods we use.

Note: I've tried to keep it as much backward compatible as possible but I'm not sure if we really need to configure things like token_prefix on the top level but if we want to have it - I'd bring it back.

Copy link
Owner

@tuwukee tuwukee left a comment

Hi @DmitryTsepelev!

Thank you, that's a nice work!

But at the moment the main goal is to get rid of extra dependency (redis), and not to introduce a new one (mock_redis). So far as we're using a pretty limited subset of redis functionality it should be possible to build a simple memory store from scratch.

@DmitryTsepelev
Copy link
Contributor Author

@DmitryTsepelev DmitryTsepelev commented Oct 21, 2018

Hi @tuwukee, thanks for the review!

I've added a MemoryStore (+ tests) implementing all the Redis methods which are in use. Also, I've removed ||= from adapters and added it to the JWTSessions#token_store method to make testing easier.

Copy link
Owner

@tuwukee tuwukee left a comment

Hi @DmitryTsepelev,

thanks, the changes look great!
Added a few comments, please take a look.


DEFAULT_REDIS_HOST = '127.0.0.1'
DEFAULT_REDIS_PORT = '6379'
DEFAULT_REDIS_DB_NAME = '0'
DEFAULT_TOKEN_PREFIX = 'jwt_'

This comment has been minimized.

@tuwukee

tuwukee Oct 22, 2018
Owner

Removal of token prefix breaks backward compatibility, please bring it back.

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Oct 22, 2018
Author Contributor

👌


module JWTSessions
module CamelizedString
refine String do

This comment has been minimized.

@tuwukee

tuwukee Oct 22, 2018
Owner

I'd prefer to not use refinements, it's rather a questionable feature.

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Oct 22, 2018
Author Contributor

👍

lib/jwt_sessions.rb Outdated Show resolved Hide resolved
test/units/jwt_sessions/test_memory_store.rb Outdated Show resolved Hide resolved
@@ -7,6 +7,7 @@ class TestRefreshToken < Minitest::Test
attr_reader :csrf, :token, :access_uid

def setup
JWTSessions.token_store = :redis

This comment has been minimized.

@tuwukee

tuwukee Oct 22, 2018
Owner

As a side note: there're kind of integration tests, defined within sinatra/rails dummy apps.
It'd be great to run them with memory store (and generic unit tests also).
Can we extend the Rakefile with rake tasks to run the same test suites, but using memory store as default?

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Oct 22, 2018
Author Contributor

working on it

lib/jwt_sessions.rb Outdated Show resolved Hide resolved
@DmitryTsepelev
Copy link
Contributor Author

@DmitryTsepelev DmitryTsepelev commented Oct 23, 2018

@tuwukee I think I've addressed everything we've discussed, please let me know if you see something else, thanks :)

@@ -7,7 +7,6 @@ class TestJWTSessions < Minitest::Test
def test_default_settings
assert_equal JWTSessions::DEFAULT_REDIS_HOST, JWTSessions.redis_host
assert_equal JWTSessions::DEFAULT_REDIS_DB_NAME, JWTSessions.redis_db_name
assert_equal JWTSessions::DEFAULT_TOKEN_PREFIX, JWTSessions.token_prefix

This comment has been minimized.

@tuwukee

tuwukee Oct 23, 2018
Owner

no need to skip this test

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Oct 23, 2018
Author Contributor

👍

Rakefile Show resolved Hide resolved
module StoreAdapters
class AbstractStoreAdapter
class << self
def instance_by_symbol(adapter, options = {})

This comment has been minimized.

@palkan

palkan Oct 23, 2018
Contributor

What about defining registry-related method on StoreAdapter module itself? Defining them on abstract adapter class makes things like RedisStoreAdapter.instance_by_symbol(...) possible, which looks strange.

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Oct 23, 2018
Author Contributor

agreed, done

Object.const_get(adapter_class_name).instance(options)
end

def instance(token_prefix: nil, **store_options)

This comment has been minimized.

@palkan

palkan Oct 23, 2018
Contributor

instance implies singleton pattern, which we do not use here (and don't need); let's use a different name, say, build

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Oct 23, 2018
Author Contributor

done

new(@_tokens_store, @_token_prefix)
end

private

This comment has been minimized.

@palkan

palkan Oct 23, 2018
Contributor

No need to override new to make it private. Just mark it:

Suggested change
private
private :new

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Oct 23, 2018
Author Contributor

done

end

def fetch_refresh(uid, namespace)
keys = %i[csrf access_uid access_expiration expiration]

This comment has been minimized.

@palkan

palkan Oct 23, 2018
Contributor

Don't we want to move this into a constant and freeze it to avoid allocations?

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Oct 23, 2018
Author Contributor

done

class MemoryStoreAdapter < AbstractStoreAdapter
class << self
def configured_store(options)
raise ArgumentError, "unknown keyword: #{options.keys.first}" if options.any?

This comment has been minimized.

@palkan

palkan Oct 23, 2018
Contributor

Let's make error message more descriptive, e.g.: "Memory store doesn't support any options"

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Oct 23, 2018
Author Contributor

👍

module StoreAdapters
class AbstractStoreAdapter
class << self
def instance_by_symbol(adapter, options = {})

This comment has been minimized.

@palkan

palkan Oct 23, 2018
Contributor

Maybe, instance_by_name? We can use strings too, not only symbols

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Oct 23, 2018
Author Contributor

👍


module JWTSessions
module StoreAdapters
def self.build_by_name(adapter, options = {})

This comment has been minimized.

@palkan

palkan Oct 23, 2018
Contributor

Let's extract it into lib/jws_sessions/store_adapters; this logic doesn't belong to abstract adapter.

We can also move all the require 'jwt_sessions/store_adapters/*_store' lines from jwt_sessions.rb there.

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Oct 24, 2018
Author Contributor

done, also removed some extra namespacing

@@ -0,0 +1,107 @@
# frozen_string_literal: true

require 'redis'

This comment has been minimized.

@palkan

palkan Oct 23, 2018
Contributor

The purpose of abstractizing store adapters is to get rid of required deps.

So, we should not depend on redis. It becomes optional.

We should only require it if Redis adapter is used. It's up to user to make sure that redis gem is available.

We can make this trick from Rails to show a meaningful error to user if redis gem couldn't be found.

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Oct 24, 2018
Author Contributor

cool! moved require 'memory_story' to the corresponding file too

class RedisStoreAdapter < AbstractStoreAdapter
class << self
def configured_store(redis_url: JWTSessions.redis_url)
redis_url ||= JWTSessions.redis_url

This comment has been minimized.

@palkan

palkan Oct 23, 2018
Contributor

Suggested change
redis_url ||= JWTSessions.redis_url

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Oct 24, 2018
Author Contributor

👍

end

def persist_refresh(uid, access_expiration, access_uid, csrf, expiration, namespace = '')
ns = namespace || ''

This comment has been minimized.

@palkan

palkan Oct 23, 2018
Contributor

Do we still need this ns local variable or we can use namespace?

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Oct 24, 2018
Author Contributor

I've tried to do that but got TestSession#test_flush_namespaced failing, because this line makes sure we use an empty string instead of nil even if the nil was explicitly passed (test imitates this exact situation)

This comment has been minimized.

@palkan

palkan Oct 24, 2018
Contributor

Got it.

Then why not namespace = namespace || ''?

end

def persist_refresh(uid, access_expiration, access_uid, csrf, expiration, namespace = '')
ns = namespace || ''

This comment has been minimized.

@palkan

palkan Oct 24, 2018
Contributor

Got it.

Then why not namespace = namespace || ''?

lib/jwt_sessions/store_adapters.rb Show resolved Hide resolved
def token_store
RedisTokenStore.instance(redis_url, token_prefix)
@token_store ||= StoreAdapters::RedisStoreAdapter.build

This comment has been minimized.

@palkan

palkan Oct 24, 2018
Contributor

Using Redis adapter by default along with making it an optional dependency is confusing.

If understand correctly, our goal is to make it possible to use the gem without Redis at all as long as making the upgrade for existent users (who use Redis) as smooth as possible. Right?

I see the following options.

  1. Enforce users to specify token_store explicitly in the configuration:
def token_store
  return @token_store if instance_variable_defined?(:@token_store)
  raise "JWTSessions.token_store is not specified. Please, configure the gem bla-bla-bla"
end
  1. Check whether redis is could be loaded and fallback to in-memory store with warning message:
def token_store
  return @token_store if instance_variable_defined?(:@token_store)
  
  begin
    require "redis"
    self.token_store = :redis
  rescue LoadError
    warn <<~MSG
      Warning! JWTSessions uses in-memory token store.
      Since you haven't configured JWTSessions.token_store, we tried to use Redis but couldn't load the 'redis' gem.
      
      Make sure that 'redis' gem is present in your Gemfile (or configure a different store).
    MSG
    self.token_store = :memory
  end
end

@tuwukee @DmitryTsepelev WDYT?

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Oct 25, 2018
Author Contributor

I like the second option because it won't affect the existing users who use redis in their apps while the first option will force them to make an additional configuration

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Oct 25, 2018
Author Contributor

I've made an implementation (except the require because when JWTSessions.token_store = :redis is called RedisAdapter would try to do it anyway). Not sure if I can test this functionality somehow

@DmitryTsepelev
Copy link
Contributor Author

@DmitryTsepelev DmitryTsepelev commented Oct 25, 2018

One more topic to discuss - in the current implementation we have an AbstractStoreAdapter with it's subclasses. All the common logic about working with storage in encapsulated inside of it so it sends messages to the store object. This object is configured by child classes, and frankly saying it's the only one responsibility subclasses have.

store can either contain a Redis client or a memory storage, which implements all the Redis methods we use. Technically these two could have a parent abstract class, and if one day we decide to add a PostgreSQL adapter we would have to find out what methods should be implemented by looking into the memory store, add the story and one more adapter (right, with one method with store configuration). Also, if we decide to use one more Redis method - it's not super clear that we should implement something else in our custom adapters.

Summing up all the issues with the current approach:

  1. Concrete adapters just configure store
  2. No explicit interface for store
  3. Adding new adapter would require adding two classes - adapter and store

I'm thinking about the following solution: instead of calling methods on store our AbstractStoreAdapter should define it's own methods related to manipulations with store and leave them unimplemented, while concrete adapters should configure stores in a way they want and implement these methods. For instance, instead of store.expireat(key, timestamp) we could call set_expiration_for(key, timestamp), redis adapter would simply delegate everything to Redis client, while memory store would contain everything we have in the MemoryStore right now. As a result - clear interface and no parallel hierarchies (aka bridge).

@tuwukee @palkan what do you think?

@palkan
Copy link
Contributor

@palkan palkan commented Oct 25, 2018

I'm thinking about the following solution: instead of calling methods on store our AbstractStoreAdapter should define it's own methods related to manipulations with store and leave them unimplemented, while concrete adapters should configure stores in a way they want and implement these methods.

+1
Makes sense.

@tuwukee
Copy link
Owner

@tuwukee tuwukee commented Oct 25, 2018

Cool, removal of unnecessary abstractions is a nice idea.

@DmitryTsepelev
Copy link
Contributor Author

@DmitryTsepelev DmitryTsepelev commented Oct 25, 2018

Done! Also, I've added some notes about configuring store adapter to the readme

README.md Show resolved Hide resolved
@DmitryTsepelev
Copy link
Contributor Author

@DmitryTsepelev DmitryTsepelev commented Oct 29, 2018

@tuwukee @palkan I think I've addressed everything we've discussed so far, so I've squashed everything into a single commit. Let me know if you see something I could improve, thanks!

@DmitryTsepelev
Copy link
Contributor Author

@DmitryTsepelev DmitryTsepelev commented Oct 29, 2018

As a followup for #39 (comment), I've redesigned API for adapters.

Now each descendant has to implement the following public methods:

  • fetch_access
  • persist_access
  • fetch_refresh
  • persist_refresh
  • update_refresh
  • all_in_namespace
  • destroy_refresh
  • destroy_access

Also, the following protected methods should be implemented too:

  • configure_store (I can't imagine an adapter which does not need a post-initialization hook configure_store)
  • wildcard_refresh_key (used in refresh_key)
@palkan
Copy link
Contributor

@palkan palkan commented Oct 29, 2018

I can't imagine an adapter which does not need a post-initialization hook configure_store

Why can't we use #initialize? (and let adapter decide whether it needs prefix or not, for example, in-memory adapter doesn't need it, does it?)

@DmitryTsepelev
Copy link
Contributor Author

@DmitryTsepelev DmitryTsepelev commented Oct 30, 2018

Okay, prefix is left read-only at the top level and concrete adapters can define it if needed, all the configuration happens inside the initialize. Also, I've removed the part that made new private - it's not a public API anymore, and the user has to use symbols anyway.

Copy link
Owner

@tuwukee tuwukee left a comment

looks good to me

store.persist_refresh('uid', expiration, 'access_uid', 'csrf', expiration, 'ns')
store.persist_refresh('uid', expiration, 'access_uid', 'csrf', expiration, 'ns2')
refresh_tokens = store.all('ns')
assert_equal refresh_tokens.count, 1

This comment has been minimized.

@batizhevsky

batizhevsky Oct 31, 2018

I suppose arguments should be in the different order:
assert_equal(exp, act, msg = nil)

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Oct 31, 2018
Author Contributor

agreed, let me check all other specs

@DmitryTsepelev
Copy link
Contributor Author

@DmitryTsepelev DmitryTsepelev commented Oct 31, 2018

Rebased once again, I guess it's ready to be merged unless there are any other issues you see :)

@palkan
palkan approved these changes Oct 31, 2018
@tuwukee tuwukee changed the base branch from master to pre-release Nov 8, 2018
@tuwukee tuwukee merged commit 8d63dc2 into tuwukee:pre-release Nov 8, 2018
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants