ueberauth / guardian_db

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

How to add more columns to 'guardian_tokens' table? #109

Closed egeersoz closed 5 years ago

egeersoz commented 5 years ago

Currently, the following migration is generated during setup:

create table(:guardian_tokens, primary_key: false) do
  add(:jti, :string, primary_key: true)
  add(:aud, :string, primary_key: true)
  add(:typ, :string)
  add(:iss, :string)
  add(:sub, :string)
  add(:exp, :bigint)
  add(:jwt, :text)
  add(:claims, :map)
end

I have a multi-tenant SaaS app, and each tenant gets their own schema in Postgres. So I need to add a new tenant column to this table so that I know which tenant each token belongs to.

Is this possible?

yordis commented 5 years ago

@egeersoz create another table where you do the join with this table.

Tenants --- Has Many --- Tenant Tokens --- Belongs To --- Guardian Tokens

Let me know if you get the idea

egeersoz commented 5 years ago

Hi @yordis

I actually tried that before opening this issue. Here's the migration:

defmodule MyApp.Repo.Migrations.Guardian.UserTokens do
  use Ecto.Migration

  def change do
    create table(:user_tokens) do
      add :jti, references(:guardian_tokens, column: :jti)
      add :user_id, :integer
      add :tenant, :string

      timestamps(type: :utc_datetime_usec)
    end
  end
end

Here's the resulting error:

(Postgrex.Error) ERROR 42830 (invalid_foreign_key) there is no unique constraint matching given keys for referenced table "guardian_tokens"

Here's what I ended up doing:

Added add(:tenant, :text) to guardian_tokens migration.

Added the following in my context:

  def update_token_with_tenant(tenant, sub, jwt) do
    query = from g in GuardianToken,
            where: g.jwt == ^jwt and g.sub == ^sub
    Repo.update_all(query, set: [tenant: tenant])
  end

Added the above function call inside after_encode_and_sign callback:

def after_encode_and_sign(resource, claims, token, _options) do
    with {:ok, {_resource, _type, _claims, jwt}} <- Guardian.DB.after_encode_and_sign(resource, claims["typ"], claims, token) do
      Accounts.update_token_with_tenant(claims["tenant"], claims["sub"], jwt)
      {:ok, token}
    end
  end

After doing all this, I can delete all of the user's tokens (e.g. when disabling their account in my app's admin console), with this:

def delete_all_tokens(tenant, user_id) do
    sub = to_string(user_id)
    query = from g in GuardianToken,
            where: g.sub == ^sub and g.tenant == ^tenant
    Repo.delete_all(query)
  end

This seems to work, but I haven't deployed it to production yet.

Anyways, the reason I opened this issue is that I feel that this library can handle multi-schema databases better, specifically the scenario where the 'guardian_tokens' table will be listed under the public schema, but the users that are referred to in the sub column are in tables under tenant schemas. I already encode the tenant in the claims, so that part works fine, but we also need to be able to query the 'guardian_tokens' table based on the tenant, and Postgres doesn't have a performant way to query JSON (e.g. JSON properties aren't indexed).

I propose this issue be reopened so we can use it for discussion.

yordis commented 5 years ago

(Postgrex.Error) ERROR 42830 (invalid_foreign_key) there is no unique constraint matching given keys for referenced table "guardian_tokens"

Based on the migration https://github.com/ueberauth/guardian_db/blob/f1328696d6d2505c35379a8e1bc3b0e98f2b2a42/priv/templates/migration.exs.eex#L6-L7 it seems that the key is a combination.

Anyways, the reason I opened this issue is that I feel that this library can handle multi-schema databases better,

I don't believe that adding more complexity to the package based on specific business use case like your is not good. The table is normalized enough to allow people to extend the database relationship. The intention of this package is not to take those business decisions.

What you did is just fine to do for those who need it to do this.

Everyone does the multi-tenant in their way, and I am really against to take a decision based on your setup.

egeersoz commented 5 years ago

I don't think the way I did it is "good", because now each time a token is issued, I perform two queries, once to insert the token, and then go back and update its tenant column in 'guardian_tokens'. There needs to be a way to do this using a single query, i.e. when GuardianDB performs the initial insert.

Regarding your broader point, in my opinion you should consider making this library flexible enough to accommodate different use cases, rather than building it for the most basic use case, with incorrect or incomplete assumptions about the user's database design, and expecting them to come up with clever hacks or complex workarounds to make it do what they need.

In addition, my use case is not rare at all. Indeed, multi-schema databases are the recommended way to do multi-tenancy in Postgres. This is how various multi-tenancy libraries like Apartmentex (based on Ruby's Apartment gem), Tenantex and Triplex work. If you take a quick look at those, you'll note that every single one uses a multi-schema approach for Postgres. GuardianDB, as it currently stands, is not flexible enough to accommodate that approach.

yordis commented 5 years ago

PR welcome, that would help to keep the discussion based on the implementation.

The main issue is that you are crossing boundaries just because you can reach for the database table, that is the whole problem of having shared database for services, the first thing people do is to reach out for the database to bypass the services.

Sorry, just because you can don't mean you should.

I totally understand your point of views, but use the service boundaries for what you are doing.