Sorcery doesn't play nice with encrypted attributes (Lockbox)

wout commented 3 years ago

Configuration

Sorcery Version: 0.16.1
Ruby Version: 3.0.1
Framework: Rails 6.1.3.2
Platform: macOS 11.4

Problem

I'm using Lockbox to encrypt sensitive information on my User model. The included columns are:

email
last_login_from_ip_address

When Sorcery is interacting with them, both raise a similar error:

ActiveRecord::StatementInvalid (PG::UndefinedColumn: ERROR:  column users.email does not exist
LINE 1: SELECT "users".* FROM "users" WHERE "users"."email" ...

That's because Lockbox uses different column names:

email -> email_ciphertext
last_login_from_ip_address -> last_login_from_ip_address_ciphertext

Sorcery does not use the standard ActiveRecord methods on User to locate it by email or update the value of last_login_from_ip_address, so both fail.

Temporary fix

For the time being, I've worked around the issue using two monkey patches. Here's the one for email:

module Sorcery
  module Adapters
    class ActiveRecordAdapter < BaseAdapter
      class << self
        def find_by_credentials(credentials)
          User.find_by(email: credentials&.first&.downcase)
        end
      end
    end
  end
end

And this one is for last_login_from_ip_address:

module Sorcery
  module Model
    module Submodules
      module ActivityLogging
        module InstanceMethods
          def set_last_ip_address(ip_address)
            update(last_login_from_ip_address: ip_address)
          end
        end
      end
    end
  end
end

What would be a more future-proof solution? I'm sure there are other areas with failures, but I didn't run into them yet.

joshbuker commented 3 years ago

I've been considering removing the ORM Adapter abstraction and depending directly on the rails ORM methods, and this leads me to believe that might be the correct path. This would also remove one of the few areas that is hard to modify for developers, which is one of the core tenants of the Sorcery design (should be easy to modify). I'd imagine most other ORM replacements should overload the same methods, making them drop-in as well.

I would say for v0, the monkey patch solution should be sufficient. I'll look into removing the ORM adapter layer for v1.

wout commented 3 years ago

I agree that would make Sorcery more compatible with other gems. I don't fully understand the use of the Adapter abstraction anyway, but I'm sure it has its use cases.

Thanks for your quick reply!

joshbuker commented 3 years ago

So it seems like the ORM adapter is mostly beneficial when it comes to defining the fields for MongoID, and handling before/after commit callbacks (which don't exist for Mongo, and need to be modified to be create/save instead of commit).

One potential route we could take is simply keeping the ORM adapters, but reducing the complexity, and perhaps extracting the find_by type methods out of the adapter while keeping the conditional things like callbacks and field definitions.

Another option is to rework the abstraction to be more flat (aka, have a method directly on the model that does the conditional checks). One downside to this option however, is that it potentially doesn't provide a clean way to differentiate applications that are using ActiveRecord vs Mongo.

Personally, I think the flat approach would be preferable because it makes overloading much easier for downstream apps, but only if a clean method to specify which ORM you're using exists. If it ends up being some messy solution (like checking if field is defined on the model, and providing some sort of override if you define field on your own and don't want it to assume you're using mongo...) I'll probably just stick to simplifying the ORM adapter as much as possible, but keeping it around for those particular pieces.

Sorcery / sorcery