ruby-passkeys / devise-passkeys

Devise extension to use passkeys instead of passwords
MIT License
161 stars 12 forks source link

Change error on unimplemented party setter #27

Closed Vagab closed 1 year ago

Vagab commented 1 year ago

I think NotImplementedError with class name reads better. Also, the message was saying relying_party which is confusing in this context, since even if it's implemented we'll still get an error.

On a separate note, @tcannonfodder, why don't we just write a default implementation in, say, Warden::WebAuthn::StrategyHelpers? I don't think many users will have a better use for it than

def set_relying_party_in_request_env
  request.env[relying_party_key] = relying_party
end

Happy to make a pr

tcannonfodder commented 1 year ago

On a separate note, @tcannonfodder, why don't we just write a default implementation in, say, Warden::WebAuthn::StrategyHelpers? I don't think many users will have a better use for it than


def set_relying_party_in_request_env

  request.env[relying_party_key] = relying_party

end

Ack, I know I had this conversation with @heliocola, but I can't find it 😬

The gist is that I don't want this gem to be too magical, which is one of the problems I have with devise as a whole.

Since the relying party is the core for the entire WebAuthn flow; the app should be responsible for:

I know it can feel like a bit of overkill, but it has the benefits of:

Vagab commented 1 year ago

The gist is that I don't want this gem to be too magical, which is one of the problems I have with devise as a whole.

totally agree with this statement. However, I think this is the perfect candidate(there are few) to actually have a generic implementation for. Mainly because I don't see how the implementation for it could reasonably be any different.

tcannonfodder commented 1 year ago

Mainly because I don't see how the implementation for it could reasonably be any different.

Hmmm; let me think on that, you make a good point.

I'm also curious about those other places you think a generic implementation would be useful; feel free to make an issue!