search

ueberauth / guardian_db

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

Update Adapter and implement EctoAdapter #129

Closed doomspork closed 3 years ago

doomspork commented 3 years ago

Howdy @yordis / @alexfilatov :wave: After reviewing #128 and looking to implement an adapter for ETS I ran into some snags that I should have caught in the first PR so I wanted to open this for the sake of conversation so we could identify if these are issues and if so, the best way to resolve them.

The first issue that I noticed when I went to implement the ETS adapter was that behaviour we setup relies pretty heavily on Ecto. If you're not working in Ecto having to deconstruct Ecto queries to get sensible data from them is not desirable. I propose we revisit the adapter behaviour and consider how we could reduce the dependency on Ecto, for this PR I updated the behavior in 4 ways:

  1. I updated all the callbacks to take the opts as pretty much any adapter will need configuration passed into it per function
  2. I updated one/2 to take in the claims rather than a query.
  3. I replaced delete_all/2 with delete_by_sub/2 to delete by subject. The original function took a query for either sub or purge, I thought it might be best to break this up.
  4. I added purge_expired_tokens to handle the second part of the aforementioned point.

Other changes I made in this PR to discuss:

I implemented the Ecto adapter and updated Token to rely on the adapter to do more work.

As-is this still does rely on Ecto Changesets being passed into the adapter. That's not as bad as queries but I'm not sure that is desirable either. Thought?

doomspork commented 3 years ago

That's a great idea @alexfilatov :+1: I think sensible defaults that help folks migrate to the newer way of doing things is always best :+1:

doomspork commented 3 years ago

@yordis / @alexfilatov We also need to update the README to mention this adapter and reflect the changes this PR (or others) might make (i.e running migrations is only relevant if you're using Ecto for instance).

yordis commented 3 years ago

But in this case if we took out behaviour from the project it will stop working.

@alexfilatov related to this https://github.com/ueberauth/guardian_db/pull/128#pullrequestreview-660388817

I want to say something about it, we haven't released anything to Hex, so nothing that has been done so far is set in stone until we release it.

yordis commented 3 years ago

@doomspork you pretty much read my mind, after I finished the PR I wonder if the abstraction should it been around the Repo or the entire thing and raise up the adapter to the right place.

I am all for this!

Related to Adapter vs Repo. Which doesn't matter much, the point I tried to make was "Adapter" refers to the design pattern, but wasn't clear about "Adapter of what?"

The thing we swap is the repository, the design pattern we follow is the adapter pattern for the repository.

Does that make sense to you?

Being said, I don't mind either way.

doomspork commented 3 years ago

Related to Adapter vs Repo. Which doesn't matter much, the point I tried to make was "Adapter" refers to the design pattern, but wasn't clear about "Adapter of what?"

But both Repository and Adapter refer to their design patterns. It sounds like StorageAdapter would address the confusion.

The thing we swap is the repository, the design pattern we follow is the adapter pattern for the repository.

We aren't swapping the repository though, we're swapping the entire storage mechanism. We're not saying it must be replaced by an Ecto.Repo or anything implementing a repository pattern which is why I think Repo is not the right choice here.

doomspork commented 3 years ago

@yordis could you give this one more look? I pushed up a commit just before you approved. I want to make sure you're cool with those changes too :+1:

yordis commented 3 years ago

But both Repository and Adapter refer to their design patterns. It sounds like StorageAdapter would address the confusion.

Exactly. This is why I was proposing to be closer to "repo" than "adapter" since it hinted to be about storage thingy.

I love that change thou, a better answer to what I tried to convey, thanks a lot!