ruby-passkeys / warden-webauthn

A Warden Strategy for WebAuthn
MIT License
19 stars 3 forks source link

Add `Warden::WebAuthn::RackHelpers`; prepend to `StrategyHelpers` #5

Closed tcannonfodder closed 1 year ago

tcannonfodder commented 1 year ago

(@Vagab I can't officially tag you as a reviewer on this, but would definitely appreciate your feedback since we've been working on this)

This closes #4

tcannonfodder commented 1 year ago

Prepending is so that the rack helper ends up in the final destination when the strategy is included, see: https://stackoverflow.com/a/15384720

We still need the old relying_party method because it's strategy-specific; while the RackHelper is meant to be used across Rack applications

Vagab commented 1 year ago

~I know the difference between prepend and append perfectly well, but what’s the situation when the method from the strategy will be called, and not from the rack helper?~

edit: I got confused and thought the RackHelpers define a relying_party which is why I was unsure. Nvm 🥲

Vagab commented 1 year ago

on the other hand, I might be sleepy or smth, but I don't get it:

  1. what's the reason for prepending vs including in the strategy helpers? Let me explain:
    
    module A; end
    module B; include A; end
    class C; include B; end

C.ancestors # => [C, B, A, ...]

module A1; end module B1; prepend A1; end class C1; include B1; end

C1.ancestors # => [C1, A1, B1, ...]



so the method will **always** end up in there. So what am I missing?
2. what's the situation when the `relying_party_key` method from `StrategyHelpers` will be called, instead of the method from `RackHelpers`? Any module which includes `StrategyHelpers` will have `RackHelpers` prioritised in the ancestors chain. However if it was included instead I can see how it could be reached. Or once again what am I missing?
tcannonfodder commented 1 year ago

I apologize, you're correct here: prepend and include accomplish the same thing. I wrote some sample code to confirm it at the end of this comment.

I got in the habit of using prepend in these cases because I'd been burned by inheritance problems in Ruby when trying to setup clearly defined "protocols" . Which I know isn't truly The Ruby Way, but the pattern is useful for certain cases.

Since the code works, and I can't think of a reason why prepend would cause issues; I think we'll keep it that way. But if there does turn out to be an issue, we'll get it fixed ASAP 💪


module A
  def check
    "A"
  end
end

module B
  include A

  def print_check
    puts check
  end
end

class C
  include B

  def check
    "C"
  end
end

class D < C
  def check
    "D"
  end
end

C.new.print_check
#=> "C"

D.new.print_check

module A1
  def check
    "A1"
  end
end

module B1
  prepend A1

  def print_check
    puts check
  end
end

class C1
  include B1

  def check
    "C1"
  end
end

class D1 < C
  def check
    "D1"
  end
end

C1.new.print_check
#=> "C1"

D1.new.print_check
Vagab commented 1 year ago

I got in the habit of using prepend in these cases because I'd been burned by inheritance problems in Ruby when trying to setup clearly defined "protocols" . Which I know isn't truly The Ruby Way, but the pattern is useful for certain cases. Since the code works, and I can't think of a reason why prepend would cause issues; I think we'll keep it that way. But if there does turn out to be an issue, we'll get it fixed ASAP 💪

Ok, makes sense 😄 . What about:

what's the situation when the relying_party_key method from StrategyHelpers will be called, instead of the method from RackHelpers? Any module which includes StrategyHelpers will have RackHelpers prioritised in the ancestors chain. However if it was included instead I can see how it could be reached. Or once again what am I missing?

tcannonfodder commented 1 year ago

I actually don't think that a relying_party_key method defined inside StrategyHelpers would be called in either case (see sample code below), which is a good thing in this case! We want the RackHelper to be the source for the relying_party_key, because it's a bridge module between this gem and other Rack apps.


module A
  def check
    "A"
  end
end

module B
  include A

  def check
    "bad value"
  end

  def print_check
    puts check
  end
end

class C
  include B

  def check
    "C"
  end
end

class D < C
  def check
    "D"
  end
end

C.new.print_check
#=> "C"

D.new.print_check

module A1
  def check
    "A1"
  end
end

module B1
  prepend A1

  def check
    "bad value1"
  end

  def print_check
    puts check
  end
end

class C1
  include B1

  def check
    "C1"
  end
end

class D1 < C
  def check
    "D1"
  end
end

C1.new.print_check
#=> "C1"

D1.new.print_check
#=> "D1"
Vagab commented 1 year ago

my point exactly. So why do we need it there at all, if it's not gonna be called at any point?

edit: here is my proposal. I'm pretty sure this is not going to change anything at any point