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

fix(dbAuth): Fix WebAuthn when authModelAccessor is set to a custom value #11605

Merged

Conversation

antonmoiseev
Copy link
Contributor

  • Instead of directly accessing auth model with hardcoded user() name, use authModelAccessor from DbAuthHandler options.
  • Also replaces findFirst() with findUnique() - we are querying data by the primary key. Feels semantically more correct, though may have no practical difference, I would expect Postgres to optimize query anyway if the only provided "where" criteria is PK.

@antonmoiseev
Copy link
Contributor Author

@Josh-Walker-GM Please, let me know if anything needs to be changed, I would be happy to help release it as soon as possible. Don't really want to push on this, but this is holding us a little bit back, because we are migrating an existing app to Redwood and User table is already taken for other data.

@antonmoiseev antonmoiseev changed the title Make WebAuthn work when authModelAccessor is set to a custom value fix(dbAuth): Make WebAuthn work when authModelAccessor is set to a custom value Sep 24, 2024
@antonmoiseev antonmoiseev changed the title fix(dbAuth): Make WebAuthn work when authModelAccessor is set to a custom value fix(dbAuth): Fix WebAuthn when authModelAccessor is set to a custom value Sep 24, 2024
@Josh-Walker-GM Josh-Walker-GM self-assigned this Sep 24, 2024
@Josh-Walker-GM Josh-Walker-GM added the release:fix This PR is a fix label Sep 24, 2024
@Josh-Walker-GM Josh-Walker-GM added this to the next-release milestone Sep 24, 2024
@Josh-Walker-GM Josh-Walker-GM linked an issue Sep 24, 2024 that may be closed by this pull request
1 task
where: { [webAuthnOptions.credentialFields.id]: credentialId },
})
.user()
const credential = await this.dbCredentialAccessor.findUnique({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Switching to findUnique over findFirst could potentially be breaking for someone that didn't have a unique, or some other equivalent, index on the database column.

I'm happy to make this change in a minor/patch. Not having a unique constraint on your webauthn id field feels like an underlying issue in the users code. Our generator/setup tool adds an @id on the prisma model so would include an index as needed here.

@Josh-Walker-GM Josh-Walker-GM merged commit ca25a4d into redwoodjs:main Sep 24, 2024
49 checks passed
@Josh-Walker-GM
Copy link
Collaborator

Thanks @antonmoiseev! I'll get this out in the 8.3.0 release which I expect to go out in the next 24 hours or so

Josh-Walker-GM pushed a commit that referenced this pull request Sep 24, 2024
…alue (#11605)

- Instead of directly accessing auth model with hardcoded `user()` name,
use `authModelAccessor` from DbAuthHandler options.
- Also replaces `findFirst()` with `findUnique()` - we are querying data
by the primary key. Feels semantically more correct, though may have no
practical difference, I would expect Postgres to optimize query anyway
if the only provided "where" criteria is PK.
@Josh-Walker-GM Josh-Walker-GM modified the milestones: next-release, v8.3.0 Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Bug?]: WebAuthn depends on specific User model name
2 participants