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: Throw error on user disabled and check revoked set true #1401

Merged
merged 18 commits into from
Aug 16, 2021

Conversation

xil222
Copy link
Contributor

@xil222 xil222 commented Aug 4, 2021

Add the logic to throw error in verifySessionCookie and verifyIdToken when user disabled and checkRevoked is true.
The logic might not look most efficient, but it is rather a simple approach without changing logic and breaking other tests.

RELEASE NOTE: When checkRevoked is set to true, the verifyIdToken() and verifySessionCookie() APIs now throw an error if the user record is disabled.

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.

Needs a bit of refactoring to make sure we don't call getUser() more than once. But mostly looks pretty good.

src/auth/auth.ts Outdated Show resolved Hide resolved
src/auth/auth.ts Outdated Show resolved Hide resolved
test/integration/auth.spec.ts Outdated Show resolved Hide resolved
test/integration/auth.spec.ts Outdated Show resolved Hide resolved
src/auth/auth.ts Outdated Show resolved Hide resolved
src/auth/auth.ts Outdated Show resolved Hide resolved
src/auth/auth.ts Outdated Show resolved Hide resolved
src/auth/auth.ts Outdated Show resolved Hide resolved
src/utils/error.ts Outdated Show resolved Hide resolved
test/integration/auth.spec.ts Outdated Show resolved Hide resolved
@xil222 xil222 requested review from egilmorez and removed request for lsirac August 9, 2021 23:35
@hiranya911 hiranya911 added the release:stage Stage a release candidate label Aug 10, 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.

The integration test passed for me without currentUser.reload(). Can we try that out with our CI and see what happens?

src/auth/auth.ts Outdated Show resolved Hide resolved
test/integration/auth.spec.ts Outdated Show resolved Hide resolved
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.

LGTM 👍

@xil222 xil222 removed their assignment Aug 12, 2021
@xil222 xil222 requested a review from bojeil-google August 12, 2021 06:18
src/auth/auth.ts Outdated Show resolved Hide resolved
src/auth/auth.ts Outdated Show resolved Hide resolved
src/auth/auth.ts Outdated Show resolved Hide resolved
test/integration/auth.spec.ts Outdated Show resolved Hide resolved
test/integration/auth.spec.ts Outdated Show resolved Hide resolved
test/unit/auth/auth.spec.ts Outdated Show resolved Hide resolved
test/unit/auth/auth.spec.ts Show resolved Hide resolved
test/unit/auth/auth.spec.ts Show resolved Hide resolved
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

A few style things to look at, thanks!

src/auth/auth.ts Outdated Show resolved Hide resolved
src/auth/auth.ts Outdated Show resolved Hide resolved
src/auth/auth.ts Outdated Show resolved Hide resolved
src/auth/auth.ts Outdated Show resolved Hide resolved
src/auth/auth.ts Outdated Show resolved Hide resolved
src/auth/auth.ts Outdated Show resolved Hide resolved
src/auth/auth.ts Outdated Show resolved Hide resolved
test/unit/auth/auth.spec.ts Outdated Show resolved Hide resolved
test/unit/auth/auth.spec.ts Outdated Show resolved Hide resolved
test/integration/auth.spec.ts Outdated Show resolved Hide resolved
test/integration/auth.spec.ts Outdated Show resolved Hide resolved
@xil222 xil222 requested a review from bojeil-google August 13, 2021 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:stage Stage a release candidate release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants