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(auth): Add token verification logic for emulator mode #419

Merged
merged 7 commits into from
Apr 21, 2021
Merged

feat(auth): Add token verification logic for emulator mode #419

merged 7 commits into from
Apr 21, 2021

Conversation

maku693
Copy link
Contributor

@maku693 maku693 commented Feb 20, 2021

Discussion

#409

This PR adds token verification with auth emulator for auth client, in addition to the changes introduced at #414.

@maku693
Copy link
Contributor Author

maku693 commented Feb 20, 2021

We should add integration tests for this PR, like Node.js SDK implementation. firebase/firebase-admin-node#1148

But I'm not sure how much should we add tests for this feature, and how to setup CI. any suggestions?

auth/auth.go Show resolved Hide resolved
@hiranya911 hiranya911 self-assigned this Feb 23, 2021
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @maku693. This looks pretty good overall. I've left a few comments on improving the readability a bit. Plus a couple of product questions to @yuchenshi around session cookie verification support.

auth/auth.go Outdated Show resolved Hide resolved
auth/auth.go Outdated Show resolved Hide resolved
auth/auth.go Show resolved Hide resolved
auth/auth.go Outdated Show resolved Hide resolved
auth/token_verifier.go Show resolved Hide resolved
auth/auth_test.go Outdated Show resolved Hide resolved
@maku693
Copy link
Contributor Author

maku693 commented Feb 24, 2021

Thank you for reviewing! I've fixed the points you've commented.

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

I'm at an LGTM.

@yuchenshi do you have any more feedback?

@robinmitra
Copy link

robinmitra commented Mar 12, 2021

Would be awesome to get this merged in if it all looks good, as this issue is breaking my local development flow.

@maku693
Copy link
Contributor Author

maku693 commented Apr 21, 2021

Hi @hiranya911 and @yuchenshi, do you have any plans to release this in the near future?

@hiranya911 hiranya911 merged commit 27ac52f into firebase:dev Apr 21, 2021
@hiranya911
Copy link
Contributor

I've merged the PR. It will be included in the next release (possibly in another week or so).

@zkrzyzanowski
Copy link

Hi all,
Thank you to those that worked on this and got this merged in! Is there an ETA on when the release with this in it might happen? It'd be great to be able to use the admin sdk with the local emulator, as this is breaking some of my dev workflow at the moment. Any other suggestions would be appreciated if there's a workaround.

Thank you!

@hiranya911
Copy link
Contributor

Will try and get it released this week.

@saulortega
Copy link

Hi.

@maku693 @hiranya911

Why when using emulator it ALWAYS checks if token was revoked? It makes no sense to me.

image

@maku693
Copy link
Contributor Author

maku693 commented Aug 8, 2023

@saulortega

I just implemented similar logic based on the Node.js implementation as a reference, so I don't have an exact understanding of the reasons.

Looking at the Node.js implementation, using checkRevoked || c.isEmulator seems to more accurately replicate the original implementation, but since it's using ||, either option appears to be valid.

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

Successfully merging this pull request may close these issues.

None yet

6 participants