-
Notifications
You must be signed in to change notification settings - Fork 627
Implement GetAppCheckToken API #2757
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
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (f61055ed) is created by Prow via merging commits: f65c333 e6df194. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (f61055ed) is created by Prow via merging commits: f65c333 e6df194. |
Macrobenchmark ReportAffected SDKsMeasurements are for head commit (de4fdf4). Diffing against base commit (91681dd) is working in progress.
|
/test check-changed |
/test macrobenchmark |
...se-appcheck/src/main/java/com/google/firebase/appcheck/internal/DefaultFirebaseAppCheck.java
Show resolved
Hide resolved
...se-appcheck/src/main/java/com/google/firebase/appcheck/internal/DefaultFirebaseAppCheck.java
Show resolved
Hide resolved
Task<AppCheckTokenResult> fetchTokenFromProvider() { | ||
return appCheckProvider | ||
.getToken() | ||
Task<AppCheckTokenResult> fetchTokenResultFromProvider() { |
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.
Splitting this method in two seems like a code smell - the inner one is for Tokens, and the outer one is for TokenResults, but then you're still constructing TokenResults in the inner one in order to trigger the listener. Is there a reason (testing maybe?) that we've done it this way? I agree that splitting the listeners into the places where their insides are constructed would also be error-prone, which is why I wonder why we've done the split at all.
Edit: I see that this is probably because of refreshing. I'm still left wondering if there's some way to architect this so that it's easier for someone without all of the relevant context to still maintain this code - this seems hard.
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.
The split is because the getAppCheckToken
method needs to return an AppCheckToken
while the getToken
method needs to return an AppCheckTokenResult
. The fetchTokenFromProvider
method should do all the heavy-lifting for getting the token, updating the stored state, and notifying listeners. All fetchTokenResultFromProvider
does is wrap the result into an AppCheckTokenResult
for Firebase SDKs to consume.
Would it be more clear for me to get rid of the fetchTokenResultFromProvider
method, and handle wrapping the token into an AppCheckTokenResult
directly within getToken
?
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.
It's just weird that you have:
- a place for marshalling things into TokenResults
- a place for internal heavy lifting, which also marshalls things into TokenResults
This would be resolved if you had a helper method that takes in a list of listeners of both types and a token and then marshals when necessary. Then you just have: - a place that takes a token and marshalls it into a TokenResult
- a place that does everything internally, but only manipulates the Token itself.
That being said, all we'd be doing is hiding the fact that we're duplicating work, so maybe that's not better either. I just know that the whole "have two Listener classes that are almost, but not quite, the same everywhere" is (IMO) just asking for a bug at some point where someone includes one but not both because we haven't isolated the pattern of dealing with both kinds of listeners to a single place. But that might be more trouble than it's worth, I leave it to you. I'm just trying to flag the concern - if I thought I had a clearly better way to do it, I'd say so, but I don't.
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.
Ack, for this PR I'll just inline the AppCheckTokenResult
wrapping into getToken
so we don't have two separate methods that do nearly the same thing.
In a follow-up PR, I'll do some refactoring to wrap the two lists of listeners into an internal class, so the logic to deal with the listeners can be encapsulated in one place.
...base-appcheck/src/main/java/com/google/firebase/appcheck/internal/DefaultTokenRefresher.java
Outdated
Show resolved
Hide resolved
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 added a bunch of comments - this might be the best way to do things, but it has some vestiges of an implementation in Auth that I regret, so I've left some thoughts.
/test smoke-tests |
Implement API for 3P developers to request a Firebase App Check token as well as observe changes to the token. This will enable developers to protect their own (non-Firebase) backends with the FAC token.