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

feat: Implement verify method #48

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

Conversation

KKimj
Copy link
Contributor

@KKimj KKimj commented Jul 30, 2024

Related Issues

fixes #20 #33
Thanks to @tkfinitefield

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.

@rrousselGit
Copy link
Collaborator

As mentioned in the original PR, we'd want tests :)

@KKimj
Copy link
Contributor Author

KKimj commented Aug 1, 2024

@rrousselGit

I'm having difficulty designing test scenarios when trying to write test code.
I think testing the process of encoding and decoding a dummy JWT token might be a good approach, but I'm curious to hear your thoughts.

If you have the time, I'd be interested in collaborating on writing the tests.
Thanks!

'no-matching-kid-error',
);
}
JWT.verify(
Copy link

Choose a reason for hiding this comment

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

Looking at what needs to be verified from https://firebase.google.com/docs/auth/admin/verify-id-tokens#verify_id_tokens_using_a_third-party_jwt_library:

  • audience: already verified in FirebaseTokenVerifier._verifyContent()
  • issuer: already verified in FirebaseTokenVerifier._verifyContent()
  • sub: presence already verified in FirebaseTokenVerifier._verifyContent()
  • exp: unless I missed something, doesn't seem to be verified, but JWT.verify() will take care of it (this should be documented because for the sake of consistency we would have wanted to verify it in _verifyContent())
  • iat: doesn't seem to be verified either, JWT.verify() has the issueAt parameter but the way it handles it and verifies the token's iat confuses me, so I would actually recommend checking it in _verifyContent()

So in summary, the use of JWT.verify() without any parameter in the proposed implementation is consistent with the existing implementation, assuming 'iat' is verified in _verifyContent().

I would recommend removing the existing verifyJwtSignature() function and the commented-out line that invokes it.

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.

Auth#verifyIdToken always throws unimplemented error
4 participants