-
Notifications
You must be signed in to change notification settings - Fork 4
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
[PM-7143] Add support for provider side forced uv to support password reprompt #5
[PM-7143] Add support for provider side forced uv to support password reprompt #5
Conversation
2dd0201
to
6c153c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some general comments, I tried to solve some of the Send+Sync issues by adding some extra bounds. Got it compiling but some tests fail, here's the diff:
diff --git a/passkey-authenticator/src/credential_store.rs b/passkey-authenticator/src/credential_store.rs
index 060c6da..15ce765 100644
--- a/passkey-authenticator/src/credential_store.rs
+++ b/passkey-authenticator/src/credential_store.rs
@@ -14,7 +14,7 @@ use passkey_types::{
#[async_trait::async_trait]
pub trait CredentialStore {
/// Defines the return type of find_credentials(...)
- type PasskeyItem: TryInto<Passkey> + Clone;
+ type PasskeyItem: TryInto<Passkey> + Send + Clone;
/// Find all credentials matching the given `ids` and `rp_id`.
///
diff --git a/passkey-client/src/lib.rs b/passkey-client/src/lib.rs
index 8654d62..1898fb9 100644
--- a/passkey-client/src/lib.rs
+++ b/passkey-client/src/lib.rs
@@ -103,7 +103,7 @@ where
S: CredentialStore + Sync,
U: UserValidationMethod + Sync,
P: public_suffix::EffectiveTLDProvider + Sync + 'static,
- Passkey: TryFrom<<S as CredentialStore>::PasskeyItem>,
+ Passkey: TryFrom<<S as CredentialStore>::PasskeyItem> + Send + Clone,
{
authenticator: Authenticator<S, U>,
rp_id_verifier: RpIdVerifier<P>,
@@ -113,7 +113,7 @@ impl<S, U> Client<S, U, public_suffix::PublicSuffixList>
where
S: CredentialStore + Sync,
U: UserValidationMethod + Sync,
- Passkey: TryFrom<<S as CredentialStore>::PasskeyItem>,
+ Passkey: TryFrom<<S as CredentialStore>::PasskeyItem> + Send + Clone,
{
/// Create a `Client` with a given `Authenticator` that uses the default
/// TLD verifier provided by `[public_suffix]`.
@@ -128,9 +128,9 @@ where
impl<S, U, P> Client<S, U, P>
where
S: CredentialStore + Sync,
- U: UserValidationMethod + Sync,
+ U: UserValidationMethod<PasskeyItem = <S as CredentialStore>::PasskeyItem> + Send + Sync,
P: public_suffix::EffectiveTLDProvider + Sync + 'static,
- Passkey: TryFrom<<S as CredentialStore>::PasskeyItem>,
+ Passkey: TryFrom<<S as CredentialStore>::PasskeyItem> + Send + Clone,
{
/// Create a `Client` with a given `Authenticator` and a custom TLD provider
/// that implements `[public_suffix::EffectiveTLDProvider]`.
mockall = { version = "0.11", optional = true } | ||
typeshare = "1" | ||
idna = "0.5" | ||
url = "2" | ||
coset = "0.3" | ||
tokio = { version = "1", features = ["sync"], optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
point-of-interest: I'm not exactly sure why I had to add mockall
and tokio
to passkey-client
, but it was the only way I could get it to compile after I added #[tokio::test]
in authenticator.rs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason they need to be added is that they are declared as dependencies for the two added features:
passkey-rs/passkey-client/Cargo.toml
Lines 18 to 19 in cea73a2
tokio = ["dep:tokio"] | |
testable = ["dep:mockall"] |
Those features seem to me like they aren't used anywhere in the passkey-client
crate so they can be removed and that means tokio
and mockall
can be removed from the [dependencies]
section.
mockall still needs to be in [dev-dependencies]
as it's used here:
passkey-rs/passkey-client/src/tests/mod.rs
Lines 62 to 64 in cea73a2
mockall::predicate::always(), | |
mockall::predicate::eq(true), | |
mockall::predicate::eq(true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm weird, passkey-authenticator also has them as normal optional dependencies too, so I guess it makes sense to follow that established pattern anyway.
`mockall` supports `async_trait` but the `#[automock]` attribute must appear before the #[async_trait] attribute. For more information see: https://docs.rs/mockall/latest/mockall/#async-traits
0e2a41e
to
cea73a2
Compare
cea73a2
to
e7703a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one small nit
…ationMethod`