-
Notifications
You must be signed in to change notification settings - Fork 301
Refactoring the FirebaseAuth Token Verification #246
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
Conversation
|
Resolves #65 |
|
|
||
| private CallableOperation<FirebaseToken, FirebaseAuthException> verifySessionCookieOp( | ||
| final String cookie, final boolean checkRevoked) { | ||
| final String cookie, boolean checkRevoked) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can continue being final ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see some more variables in here that can be finalized. Can we change across the board?
src/main/java/com/google/firebase/auth/RevocationCheckDecorator.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/firebase/auth/RevocationCheckDecorator.java
Outdated
Show resolved
Hide resolved
|
Thanks @ashwinraghav. I've addressed your comments. Please take a look. |
ashwinraghav
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor Nit. Otherwise LGTM
|
Lot of these methods used to have anonymous inner classes that wrap the arguments. Which is why they are marked |
The implementation of
FirebaseAuthfor the most part is a monolith without clear separation of concerns. It is currently tightly coupled with theFirebaseTokenFactory,FirebaseTokenVerifierandFirebaseUserManagerconcrete classes. Because of this it is difficult to make changes to the individual helper classes, and also it is difficult to properly unit test theFirebaseAuthAPI surface. This is evident from the lack of test coverage in the following classes/packages:This is the first of a series of refactorings I've planned in order to make the implementation more modular and more testable. In this PR I'm mostly focusing on the token verification logic, while keeping user management and custom token creation intact. The key changes include:
FirebaseTokenVerifieris now an interface. This dependency is injected into theFirebaseAuthvia a a builder. This makes it possible to test all token verification APIs by using a mockFirebaseTokenVerifierin unit tests.FirebaseTokenVerifierImplclass provides the default implementation ofFirebaseTokenVerifier. All uses of theIdTokenAPI are handled by this class as implementation details. This encapsulation enables us to easily change how we parse/verify ID tokens in the future (if the need ever arises).FirebaseTokenVerifierinterface, and engaged as needed via the decorator pattern.FirebaseTokenimplementation has been decoupled from the Google API client'sIdTokenimplementation.Supplier<T>interface to inject token verifier dependencies intoFirebaseAuth. In addition to dependency injection, this provides us an elegant way to implement lazy loading as well.Test coverage numbers after this change: