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

Question: Can the User Pool also support the USER_PASSWORD_AUTH flow for classic signin/signup? #138

Open
harrygreen opened this issue Jan 14, 2024 · 18 comments

Comments

@harrygreen
Copy link

harrygreen commented Jan 14, 2024

Hey. The library is fantastic. Perhaps this is more related to Cognito than specifically to this library.

We've successfully deployed the CDK setup and now want to allow a standard password auth flow for signin and signup. Password will be a fallback option for signin, and a default option for signup.

  1. To achieve that, we've manually added USER_PASSWORD_AUTH in the user pool.
  2. We've enabled self-registration for signups (with password).
  3. After the user is authenticated, we prompt them to add the Fido2 credentials via fido2CreateCredential().

Are there any problems with this? If not, I'm happy to help with a PR to expose these options.

@ottokruse
Copy link
Contributor

The library is fantastic.

Thanks for that!

The flow you wrote down should work, and should be supported by the library already. Well to be more exact, it is supported by the backend and front end library code (authenticateWithSRP and authenticateWithPlaintextPassword, use SRP if you can), but the prefab React components do not use that––but I presume you are building your own components?

We've been playing with the idea of adding username+password auth to the React component too, as another option besides magic link and paskey, but haven't done so yet.

@harrygreen
Copy link
Author

harrygreen commented Jan 15, 2024

Hey @ottokruse. Thanks for the reply.

We're actually trying to continue using as much of the Amplify UI React screens as possible, rather than rewriting them for integration with this library. Is that what you mean by "prefab React components"?

Promisingly, the two libraries seem to working together well so far: the CUSTOM_AUTH lambdas are invoked with usePasswordless().authenticateWithFido2(), and password flow is invoked with the existing Amplify UI form.

But I now have an issue with Unrecognized RP ID when attempting to login with CUSTOM_AUTH from http://localhost:5173. I'm not sure if that's a related issue.

Here's the config:

Passwordless.configureFromAmplify(
  Amplify.configure({
    Auth: {
      region: "eu-west-1",
      userPoolId: "eu-west-1_lBAOg7CvJ",
      userPoolWebClientId: "51uiujj9qmhu82dmphp3ila3rn", // Client ID for both CUSTOM_AUTH and password flows
      authenticationFlowType: "USER_PASSWORD_AUTH", // legacy but will definitely look at switching to USER_SRP_AUTH
    },
  })
).with({
  fido2: {
    baseUrl: "https://xpzygh93g8.execute-api.eu-west-1.amazonaws.com/v1/",
    authenticatorSelection: {
      authenticatorAttachment: "platform",
      userVerification: "required",
    },
    rp: {
      id: "localhost",
      name: "localhost",
    },
  },
  proxyApiHeaders: {
    "Access-Control-Allow-Origin": "*",
  },
  debug: console.debug,
});
Auth.configure();

@ottokruse
Copy link
Contributor

We're actually trying to continue using as much of the Amplify UI React screens as possible, rather than rewriting them for integration with this library. Is that what you mean by "prefab React components"?

Yes indeed. Not sure though how you're going to be able to add a button "Sign in with passkey" to the prefab Amplify UI, but I haven't looked into it maybe it is possible––would love to know!

Promisingly, the two libraries seem to working together well so far: the CUSTOM_AUTH lambdas are invoked with usePasswordless().authenticateWithFido2(), and password flow is invoked with the existing Amplify UI form.

Nice to hear that this works :) We did intend it.

Unrecognized RP ID comes from the backend: make sure to allow localhost as RP in your CDK stack like we do in the end 2 end example:

@harrygreen
Copy link
Author

Not sure though how you're going to be able to add a button "Sign in with passkey" to the prefab Amplify UI, but I haven't looked into it maybe it is possible––would love to know!

I'm currently just injecting the button into the Signin screen's header section via custom components - here's a contrived example:

const components = {
  SignIn: {
    Header() {
      return (
        <>
          <p>Sign in with biometrics:</p>
          <button onClick={() => authenticateWithFido2()} />
          <p>or with password:</p>
        </>
      )
    },
  },
};

<Authenticator components={components}>...</Authenticator>

Unrecognized RP ID comes from the backend: make sure to allow localhost as RP in your CDK stack like we do in the end 2 end example:

Thank you 🙏 I'll redeploy the stack and report back...

@ottokruse
Copy link
Contributor

Amazing! Once you have it all working I want to add an example like that as example here in this repo. Awesome stuff

@harrygreen
Copy link
Author

It works :) I can sign in with biometrics (or password), and add fido2 credentials after should the account need it. Amazing.

I'll tidy up the code and post it in this chat.

Thanks again - this is huge for us and our clients!

@ottokruse
Copy link
Contributor

Cool! 🎉

Looking forward to hearing/seeing more

@ottokruse
Copy link
Contributor

@harrygreen how has it been going?

@harrygreen
Copy link
Author

Not bad thanks @ottokruse. We're in the process of converting only the output we need - lambdas, api gateway, dynamo - into our Terraform setup.
As for the frontend, the usePasswordless hook is very useful and hasn't needed forking.

@ottokruse
Copy link
Contributor

Great news

@harrygreen
Copy link
Author

harrygreen commented Mar 7, 2024

Hey @ottokruse, quick question. If a user deletes their passkey from their device (an accepted scenario), dynamoDB doesn't know about it. So, I tried to let the user re-add their device - but I get the browser error:

The user attempted to register an authenticator that contains one of the credentials already registered with the relying party.

What would you recommend doing in this scenario? Call deleteCredentials/updateCrendetials from the consumer (frontend)?
Thanks.

@ottokruse
Copy link
Contributor

ottokruse commented Mar 7, 2024

This happens because the credential ID is the same as one that already exists in DynamoDB and is sent to the client as part of the registration ceremony in excludeCredentials:

excludeCredentials: existingCredentials.map((credential) => ({

The browser then throws that error if the user tries to register a credential with an ID that is in that list.

Catch the error and offer the user the option of removing the existing credential from the Relying Party (DynamoDB)?

"It looks like you've registered this device with us before. We must first remove it from our records, before you can register it again."

My colleague suggests: if you have a mix of resident/non-resident keys, send excludeCredentials for the non-resident keys only.

(A resident key is a discoverable credential, that supports usernameless sign-in. These are often called passkeys but that term is also used more and more for non-discoverable credentials)

So if you (we) do what my colleague suggests, we wouldn't have the error in the first place. Users can then always reregister their discoverable credentials and it will overwrite the record both on their device and on the server.

That's a great idea but AFAIK it is not easy to detect what credential is a resident key (passkey) on RP side (yet), so I can't suggest a path there.

@ottokruse
Copy link
Contributor

Was just suggested to use the credProps extension to see on RP side if a credential is a resident key

@harrygreen
Copy link
Author

harrygreen commented Mar 18, 2024

Thanks @ottokruse, that worked. I listen for InvalidStateError and handle accordingly.

We've gone live with the implementation 👍, with mixed results - we're seeing problems with tokens/sessions in the wild.
We're getting a lot of Cognito errors for Cannot retrieve a new session. Please authenticate., implying the tokens stored by this library are not expiring or refreshing as expected.

Reading the Amplify JS docs https://docs.amplify.aws/javascript/prev/build-a-backend/auth/switch-auth/#customauth-flow states authenticationFlowType: CUSTOM_AUTH is supported. And because we have this library already in use, it makes sense to me for Amplify to completely handle the tokens refresh/expiry and all https://cognito-idp... calls, and instead using this library to create the passkey creds which are passed to the CUSTOM_AUTH method via Auth.signIn. What do you think?

tl;dr
Would it be possible to use this library to login with the Fido2 credentials, and pass over to Amplify for the rest of the session management?

@ottokruse
Copy link
Contributor

ottokruse commented Mar 18, 2024

Hey mate. About Cannot retrieve a new session. Please authenticate. I think that is actually an Amplify error.

Check this search: https://github.com/search?q=%22Cannot+retrieve+a+new+session.+Please+authenticate.%22&type=code

Looks like Amplify thinks there is no refresh token? maybe upgrading to Amplify V6 helps you.

Would it be possible to use this library to login with the Fido2 credentials, and pass over to Amplify for the rest of the session management?

Yes, you shouldn't have to do anything special for it, besides loading both (they should not conflict). But maybe you are already doing that, otherwise I don't understand why you'd see an Amplify error?

Would be great to dig in why this is happening exactly

@harrygreen
Copy link
Author

harrygreen commented Mar 19, 2024

Thanks. The Cannot retrieve a new session. Please authenticate. is hard to replicate locally, but from the logs in the wild, I think it's to do with a mismatch between sessions expiring on Cognito and Amplify not expecting that to be the case. The usePasswordless hook contains a lot of token management (storage, refresh, expiry) which the Amplify JS library is also doing. So in my case I had two lots of entries in localStorage. I hadn't spotted this before, but it's a (very) red flag and could explain Amplify's confusion.

Yes, you shouldn't have to do anything special for it

Yep, it's working. I've just got Amplify's Auth.sign() plugged in with the "usernameless" code from authenticateWithFido2() https://github.com/aws-samples/amazon-cognito-passwordless-auth/blob/main/client/fido2.ts#L532-L610, eg:

const fido2options = await requestUsernamelessSignInChallenge();
const fido2credential = await fido2getCredential({
  ...fido2options,
  relyingPartyId: "localhost",
});
if (!fido2credential.userHandleB64) {
  throw new Error("No discoverable credentials available");
}
let username = new TextDecoder().decode(
  bufferFromBase64Url(fido2credential.userHandleB64)
);
if (username.startsWith("s|")) {
  throw new Error("Username is required for initiating sign-in");
}
username = username.replace(/^u\|/, "");
const user = await Auth.signIn(username, "");
if (user?.challengeName === "CUSTOM_CHALLENGE") {
  try {
    await Auth.sendCustomChallengeAnswer(
      user,
      JSON.stringify(fido2credential),
      {
        signInMethod: "FIDO2",
      }
    );
  } catch (err) {
    ...
  }
}

I'm working on the create credentials part now.

@ottokruse
Copy link
Contributor

Was hoping you wouldn't have to do that but yes you can, and that's probably the most seamless way to integrate with Amplify. Although it remains guessing why you get the error you did.

So in my case I had two lots of entries in localStorage.

Should just have 1 ID, 1 Access and 1 Refresh token though? (these are shared between this lib and Amplify)

@harrygreen
Copy link
Author

I'm trying to find the cause. @aws-amplify/ui-react is a bit of a black of box, so I can't see when it's calling the aws-amplify/amplify-js methods, but the token it creates are there before this library's storage.js > storeTokens() is called.

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

No branches or pull requests

2 participants