Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Jun 24, 2023

Conversation

tcannonfodder
Copy link
Contributor

@tcannonfodder tcannonfodder commented Jun 24, 2023

  • In order to provide a clean, isolated module for other libraries to use the centralized default relying_party_key, we need to:
    • Break out the relying_party_key into a new Warden::WebAuthn::RackHelpers module
    • Auto-include that module as part of Warden::WebAuthn::StrategyHelpers to achieve the same goal of a centrally located default key

(@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

* In order to provide a clean, isolated module for other libraries to
	use the centralized default `relying_party_key`, we need to:
	* Break out the relying_party_key into a new
		`Warden::WebAuthn::RackHelpers` module
	* Auto-include that module as part of
		`Warden::WebAuthn::StrategyHelpers` to achieve the same goal of a
		centrally located default key
* In order to streamline standard setups, we can provide the
	`set_relying_party_in_request_env` helper method; which can be
	included via `Warden::WebAuthn::RackHelpers` and used as needed
Copy link
Contributor

@Vagab Vagab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for prepending?

edit: ohh, because we want relying_party, makes sense
edit2: why do we need the old relying_party in the strategy?

@tcannonfodder
Copy link
Contributor Author

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

@tcannonfodder tcannonfodder merged commit 8c1ef20 into main Jun 24, 2023
@Vagab
Copy link
Contributor

Vagab commented Jun 24, 2023

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
Copy link
Contributor

Vagab commented Jun 24, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

Vagab commented Jun 24, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

Vagab commented Jun 24, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor relying_party_key into Warden::WebAuthn::RackHelpers
2 participants