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: return transports in attestation response pk credential source #435

Merged
merged 1 commit into from
Jul 15, 2023
Merged

fix: return transports in attestation response pk credential source #435

merged 1 commit into from
Jul 15, 2023

Conversation

joostdebruijn
Copy link
Contributor

Target branch: 4.7.x
Resolves issue: n/a

  • It is a Bug fix
  • It is a New feature
  • Breaks BC
  • Includes Deprecations

The PublicKeyCredentialSource from the AuthenticatorAttestationResponseValidator was always returning an empty array for transports. This PR fixes that and adds the transports as given in the AuthenticatorAttestationResponse. This change breaks the constructor signature of AuthenticatorAttestationResponse by adding an additional parameter.

Tests are adjusted accordingly.

@joostdebruijn
Copy link
Contributor Author

@Spomky I fixed the styling issue from the tests. However, the Rector tests that are failing are not related to any changes I made. I could fix them, but I think it's better to create a separate PR for that.

@Spomky
Copy link
Contributor

Spomky commented Jul 15, 2023

Hi,

Many thanks for this PR.
I have just pushed a minor modification:

  • To avoid BC break, the new $transports argument in the AuthenticatorAttestationResponse has a default value set to an empty array.

Rector/ECS issues will be fixed before the release of this version.

@Spomky Spomky merged commit ca754d8 into web-auth:4.7.x Jul 15, 2023
@joostdebruijn joostdebruijn deleted the fix/add-transports branch July 15, 2023 23:44
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.

2 participants