Skip to content

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

Merged
hiranya911 merged 7 commits into
firebase:devfrom
maku693:auth-emulator-token-verification
Apr 21, 2021
Merged

hiranya911 merged 7 commits into
firebase:devfrom
maku693:auth-emulator-token-verification

Conversation

@maku693

@maku693 maku693 commented Feb 20, 2021 •

Copy link
Copy Markdown
Contributor

Discussion

#409

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

@maku693

maku693 commented Feb 20, 2021

Copy link
Copy Markdown
Contributor Author

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?

Comment thread auth/auth.go
Comment thread auth/auth_test.go
Comment thread auth/token_verifier.go
@hiranya911 hiranya911 self-assigned this Feb 23, 2021

@hiranya911 hiranya911 left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread auth/auth.go Outdated
Comment thread auth/auth.go Outdated
Comment thread auth/auth.go
Comment thread auth/auth.go Outdated
Comment thread auth/token_verifier.go
Comment thread auth/auth_test.go Outdated
@maku693

maku693 commented Feb 24, 2021

Copy link
Copy Markdown
Contributor Author

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

@maku693
maku693 requested a review from hiranya911 February 24, 2021 15:30

@hiranya911 hiranya911 left a comment

Copy link
Copy Markdown
Contributor

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

robinmitra commented Mar 12, 2021 •

Copy link
Copy Markdown

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

@maku693

maku693 commented Apr 21, 2021

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

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

@zkrzyzanowski

Copy link
Copy Markdown

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
Copy Markdown
Contributor

Will try and get it released this week.

@saulortega

Copy link
Copy Markdown

Hi.

@maku693 @hiranya911

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

image

@maku693

maku693 commented Aug 8, 2023 •

Copy link
Copy Markdown
Contributor Author

@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.

6 participants