Skip to content

Commit

Permalink
Refactor ActiveRecord::TokenFor to not rely on relation delegation
Browse files Browse the repository at this point in the history
Ref: rails#50396
Ref: rails#51776

`ActiveRecord::Relation` automatically delegates missing methods
to the model class wrapped in a `scoping { }` block.

This is to support scoping in user defined class methods. The problem
however is that it's very error prone for the framework, because we
can mistakenly call model methods from inside `Relation` and not
realized we're applying a global scope.

In the best case scenario it's just a waste of performance, but
it can also lead to bugs like rails#51775

I'm planning to restrict this automatic delegation to methods defined
in childs of `ActiveRecord::Base` only: rails#50396
but for this to work we must first refactor any Rails code that rely on it.
  • Loading branch information
byroot committed May 13, 2024
1 parent 007a609 commit 9aec4a2
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 12 deletions.
1 change: 1 addition & 0 deletions activerecord/lib/active_record/relation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ def exec_explain(&block)

include Enumerable
include FinderMethods, Calculations, SpawnMethods, QueryMethods, Batches, Explain, Delegation
include TokenFor::RelationMethods

attr_reader :table, :klass, :loaded, :predicate_builder
attr_accessor :skip_preloading_value
Expand Down
34 changes: 22 additions & 12 deletions activerecord/lib/active_record/token_for.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,24 @@ def resolve_token(token)
end
end

module RelationMethods
# Finds a record using a given +token+ for a predefined +purpose+. Returns
# +nil+ if the token is invalid or the record was not found.
def find_by_token_for(purpose, token)
raise UnknownPrimaryKey.new(self) unless model.primary_key
model.token_definitions.fetch(purpose).resolve_token(token) { |id| find_by(model.primary_key => id) }
end

# Finds a record using a given +token+ for a predefined +purpose+. Raises
# ActiveSupport::MessageVerifier::InvalidSignature if the token is invalid
# (e.g. expired, bad format, etc). Raises ActiveRecord::RecordNotFound if
# the token is valid but the record was not found.
def find_by_token_for!(purpose, token)
model.token_definitions.fetch(purpose).resolve_token(token) { |id| find(id) } ||
(raise ActiveSupport::MessageVerifier::InvalidSignature)
end
end

module ClassMethods
# Defines the behavior of tokens generated for a specific +purpose+.
# A token can be generated by calling TokenFor#generate_token_for on a
Expand Down Expand Up @@ -85,20 +103,12 @@ def generates_token_for(purpose, expires_in: nil, &block)
self.token_definitions = token_definitions.merge(purpose => TokenDefinition.new(self, purpose, expires_in, block))
end

# Finds a record using a given +token+ for a predefined +purpose+. Returns
# +nil+ if the token is invalid or the record was not found.
def find_by_token_for(purpose, token)
raise UnknownPrimaryKey.new(self) unless primary_key
token_definitions.fetch(purpose).resolve_token(token) { |id| find_by(primary_key => id) }
def find_by_token_for(purpose, token) # :nodoc:
all.find_by_token_for(purpose, token)
end

# Finds a record using a given +token+ for a predefined +purpose+. Raises
# ActiveSupport::MessageVerifier::InvalidSignature if the token is invalid
# (e.g. expired, bad format, etc). Raises ActiveRecord::RecordNotFound if
# the token is valid but the record was not found.
def find_by_token_for!(purpose, token)
token_definitions.fetch(purpose).resolve_token(token) { |id| find(id) } ||
(raise ActiveSupport::MessageVerifier::InvalidSignature)
def find_by_token_for!(purpose, token) # :nodoc:
all.find_by_token_for!(purpose, token)
end
end

Expand Down

0 comments on commit 9aec4a2

Please sign in to comment.