Skip to content

fix(auth): check if the user is disabled on checkRevoked=true for verifyIdToken and verifySessionCookie #585

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 9, 2021
Merged

fix(auth): check if the user is disabled on checkRevoked=true for verifyIdToken and verifySessionCookie #585

merged 2 commits into from
Aug 9, 2021

Conversation

lsirac
Copy link
Contributor

@lsirac lsirac commented Aug 6, 2021 •
edited
Loading

When verifyIdToken or verifySessionCookie is called with checkRevoked set to true we also check if the user is disabled. If so, a FirebaseAuthException with the new AuthErrorCode USER_DISABLED is thrown.
@lsirac lsirac requested review from egilmorez and hiranya911 August 6, 2021 01:02
hiranya911
hiranya911 suggested changes Aug 6, 2021
View reviewed changes
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.

Looks pretty good. Just a few minor nits pointed out.
@lsirac lsirac requested a review from hiranya911 August 7, 2021 23:13
hiranya911
hiranya911 approved these changes Aug 9, 2021
View reviewed changes
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. LGTM 👍
@hiranya911
Copy link
Contributor

@egilmorez PTAL the javadoc updates in AbstractFirebaseAuth and AuthErrorCode.
egilmorez
egilmorez reviewed Aug 9, 2021
View reviewed changes
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.

LG, though a couple of comments don't fully make sense to me -- possibly my problem but please clarify if you can, thanks!
egilmorez
egilmorez approved these changes Aug 9, 2021
View reviewed changes
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.

Thanks!
@lsirac lsirac merged commit 2d50628 into master Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note

3 participants

@lsirac @hiranya911 @egilmorez