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 Issues with verifyIdToken #58

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fix Issues with verifyIdToken #58

wants to merge 6 commits into from

Conversation

jtdLab
Copy link

@jtdLab jtdLab commented Oct 27, 2024

Related Issues

fixes #57
fixes #56

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).

  • I have updated the CHANGELOG.md of the relevant packages.
    Changelog files must be edited under the form:

    ## Unreleased fix/major/minor
    
    - Description of your change. (thanks to @yourGithubId)
  • If this contains new features or behavior changes,
    I have updated the documentation to match those changes.

sky-rabbit-cnc added a commit to sky-rabbit-cnc/dart_firebase_admin that referenced this pull request Nov 3, 2024
try {
verifyJwtSignature(
Copy link
Contributor

@labrom labrom Nov 7, 2024

Choose a reason for hiding this comment

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

I would recommend maybe keeping the verifyJwtSignature function call, and modifying the function to only catch JWTExpiredException, since the handling of JWTExpiredException is the same in both verifiers.

@internal
factory TokenProvider.fromMap(Map<dynamic, dynamic> map) {
return TokenProvider(
identities: map['identities']! as Map<String, Object?>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since behavior changed here, would you mind writing a test?

@jtdLab
Copy link
Author

jtdLab commented Nov 8, 2024

I made the changes do we need more tests for the emulator related changes?

'phone_number': 'mock-phone-number',
'picture': 'mock-picture',
'sub': 'mock-sub',
'uid': 'mock-sub',
Copy link
Contributor

Choose a reason for hiding this comment

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

The doc says that uid isn't actually present in the token, and this field is just a convenience that takes its value from sub. How about removing this line then?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch

@labrom
Copy link
Contributor

labrom commented Nov 8, 2024

I believe this should also fix #56, would you mind updating the description?

@jtdLab
Copy link
Author

jtdLab commented Nov 8, 2024

Done

@labrom
Copy link
Contributor

labrom commented Nov 8, 2024

Awesome, thanks!
@rrousselGit will need to review.

@irshad365
Copy link

@rrousselGit can you please review and release this fix? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants