ueberauth / guardian_db

Guardian DB integration for tracking tokens and ensuring logout cannot be replayed.
MIT License
368 stars 87 forks source link

Guardian DB Redis Repo #119

Closed alexfilatov closed 3 years ago

alexfilatov commented 3 years ago

hey there! :)

First of all thanks a lot for guardian_db, saved me a lot of time!

I created a module that stores Guardian tokens in Redis, do you think this is worth a PR to guardian_db?

https://github.com/alexfilatov/guardian_db_redis

This is first version, please let me know what you think.

aleDsz commented 3 years ago

I think it's a good idea, and we can improve that using an Adapter for token storage, using the same @behaviour to allow users to change to their preference, and ur repo can become and plugin adapter for Redis.

What u think?

yordis commented 3 years ago

@alexfilatov welcome to add some adapter pattern around GuardianDB, make sure to tag me on it ... I keep losing notifications, I will help you to review and merge it

aleDsz commented 3 years ago

I was thinking about this API

defmodule Guardian.DB.Adapter do
  @moduledoc """
  The Guardian DB Adapter.

  This behaviour allows to use any storage system
  for Guardian Tokens.
  """

  @typep query :: Ecto.Query.t()
  @typep schema :: Ecto.Schema.t()
  @typep schema_or_changeset :: schema() | Ecto.Changeset.t()
  @typep queryable :: query() | schema()
  @typep opts :: keyword()
  @typep id :: pos_integer() | binary() | Ecto.UUID.t()

  @callback one(queryable()) :: nil | schema()
  @callback get(queryable(), id()) :: nil | schema()
  @callback insert(schema_or_changeset()) :: {:ok, schema()}
  @callback delete(schema_or_changeset()) :: {:ok, schema()}
  @callback delete_all(queryable()) :: {:ok, pos_integer()}
  @callback delete_all(queryable(), opts()) :: {:ok, pos_integer()}
end

@alexfilatov do you think it's enough to bring Redis, or any storage adapter to Guardian.DB?

alexfilatov commented 3 years ago

love it @aleDsz, will work on PR asap, will let you guy know when ready!

aleDsz commented 3 years ago

@alexfilatov i can help if u want to. I was thinking about to ask your help to make a good integration using this behaviour to test the idea and checking if it would work with Ecto and Redix

doomspork commented 3 years ago

This is a great idea! @alexfilatov if you want to open a draft PR to solicit feedback feel free :+1:

alexfilatov commented 3 years ago

Hello there again @doomspork and @aleDsz!

Sorry for the long reply, got back to this one eventually.

I got a slightly different idea. I started working on the PR for guardian_db but one thing was very annoying for me - why would guardian_db have some new dependencies (on redix and jason) just because of a new Redis repo? What if the user will not use Redis as storage for tokens but still have those dependencies? Also, I read a great article from @savonarola about SOLID principles in Elixir and that was literally my case!

So I started working on https://github.com/alexfilatov/guardian_redis - a plugin for guardian_db. The only thing user need to do is to include this library in deps and change repo settings (well, Redis settings as well, but this goes without saying:)

This is still a Work in Progress but I would like to hear your early feedback, please.

alexfilatov commented 3 years ago

this one solved by adding an adapter here https://github.com/ueberauth/guardian_db/pull/126 and implementing a Redis plugin https://github.com/alexfilatov/guardian_redis

yordis commented 3 years ago

@alexfilatov I forgot about something, I only see {:ok, ....} success responses in the callback, is that correct? or do we need some error tuple response as well?

alexfilatov commented 3 years ago

this is a great catch @yordis! yes, we need {:error, ... } as well, e.g.

@callback insert(schema_or_changeset()) :: {:ok, schema()} | {:error, Ecto.Changeset.t()}

Will prepare new PR.

alexfilatov commented 3 years ago

Hey @yordis, made a PR, could you please have a look? ^^