Skip to content

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

Merged
merged 7 commits into from
Jun 28, 2021
Merged

Implement GetAppCheckToken API #2757

merged 7 commits into from
Jun 28, 2021

Conversation

rosalyntan
Copy link
Member

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.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jun 23, 2021

Coverage Report

Affected SDKs

  • firebase-appcheck

    SDK overall coverage changed from 83.37% (f65c333) to 83.15% (f61055ed) by -0.22%.

    Filename Base (f65c333) Head (f61055ed) Diff
    DefaultFirebaseAppCheck.java 83.33% 82.08% -1.26%
    DefaultTokenRefresher.java 94.59% 96.97% +2.38%

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (f61055ed) is created by Prow via merging commits: f65c333 e6df194.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jun 23, 2021

Binary Size Report

Affected SDKs

  • firebase-appcheck

    Type Base (f65c333) Head (f61055ed) Diff
    aar 33.2 kB 34.5 kB +1.29 kB (+3.9%)
    apk (aggressive) 284 kB 284 kB +8 B (+0.0%)
    apk (release) 935 kB 936 kB +444 B (+0.0%)
  • firebase-appcheck-debug

    Type Base (f65c333) Head (f61055ed) Diff
    apk (aggressive) 284 kB 284 kB +8 B (+0.0%)
    apk (release) 938 kB 939 kB +392 B (+0.0%)
  • firebase-appcheck-debug-testing

    Type Base (f65c333) Head (f61055ed) Diff
    apk (aggressive) 286 kB 286 kB +8 B (+0.0%)
    apk (release) 972 kB 972 kB +376 B (+0.0%)

Test Logs

Notes

Head commit (f61055ed) is created by Prow via merging commits: f65c333 e6df194.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jun 23, 2021

Macrobenchmark Report

Affected SDKs

Measurements are for head commit (de4fdf4). Diffing against base commit (91681dd) is working in progress.

  • baseline

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait120.0 ms165.0 ms135.0 ms146.0 ms161.4 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait31.0 ms62.0 ms54.0 ms58.3 ms61.8 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait60.0 ms79.0 ms73.5 ms78.1 ms79.0 ms
  • firebase-common

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait127.0 ms159.0 ms145.5 ms156.2 ms158.8 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait43.0 ms70.0 ms55.5 ms65.4 ms69.8 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait62.0 ms91.0 ms73.5 ms87.0 ms90.2 ms
  • firebase-config

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait132.0 ms186.0 ms149.0 ms166.4 ms183.0 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait36.0 ms62.0 ms48.0 ms59.1 ms61.6 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait53.0 ms105.0 ms69.0 ms93.0 ms102.7 ms
  • firebase-crashlytics

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait134.0 ms169.0 ms156.5 ms166.0 ms168.4 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait35.0 ms62.0 ms51.5 ms62.0 ms62.0 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait54.0 ms93.0 ms76.5 ms89.1 ms92.4 ms
  • firebase-database

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait126.0 ms154.0 ms142.0 ms146.7 ms153.8 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait37.0 ms59.0 ms52.0 ms58.0 ms58.8 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait53.0 ms90.0 ms74.0 ms84.1 ms89.1 ms
  • firebase-dynamic-links

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait130.0 ms162.0 ms145.0 ms156.1 ms161.1 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait33.0 ms65.0 ms55.0 ms64.0 ms64.8 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait50.0 ms92.0 ms67.0 ms90.1 ms91.8 ms
  • firebase-firestore

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait130.0 ms175.0 ms145.0 ms161.1 ms172.5 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait39.0 ms77.0 ms56.5 ms65.0 ms74.7 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait57.0 ms88.0 ms76.5 ms85.0 ms87.4 ms
  • firebase-functions

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait135.0 ms165.0 ms141.0 ms163.1 ms164.8 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait37.0 ms73.0 ms51.0 ms69.2 ms72.6 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait56.0 ms86.0 ms70.5 ms85.0 ms85.8 ms
  • firebase-inappmessaging-display

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait168.0 ms214.0 ms188.0 ms201.1 ms211.7 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait36.0 ms78.0 ms54.5 ms61.8 ms76.3 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait60.0 ms94.0 ms78.5 ms89.0 ms93.0 ms
  • firebase-messaging

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait153.0 ms206.0 ms175.5 ms197.5 ms205.2 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait38.0 ms69.0 ms57.5 ms66.0 ms68.4 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait65.0 ms94.0 ms80.5 ms90.1 ms93.4 ms
  • firebase-perf

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait143.0 ms174.0 ms160.0 ms165.8 ms173.8 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait47.0 ms61.0 ms54.0 ms58.3 ms61.0 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait57.0 ms91.0 ms70.5 ms88.2 ms90.8 ms
  • firebase-storage

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait134.0 ms164.0 ms146.0 ms159.5 ms164.0 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait40.0 ms73.0 ms55.5 ms61.9 ms72.4 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait50.0 ms98.0 ms80.0 ms88.4 ms96.9 ms

@rosalyntan rosalyntan marked this pull request as ready for review June 23, 2021 23:00
@rosalyntan
Copy link
Member Author

/test check-changed

@rosalyntan rosalyntan requested a review from malcolmdeck June 23, 2021 23:31
@rosalyntan
Copy link
Member Author

/test macrobenchmark

Task<AppCheckTokenResult> fetchTokenFromProvider() {
return appCheckProvider
.getToken()
Task<AppCheckTokenResult> fetchTokenResultFromProvider() {
Copy link
Contributor

@malcolmdeck malcolmdeck Jun 24, 2021

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.

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

@malcolmdeck malcolmdeck left a 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.

@rosalyntan
Copy link
Member Author

/test smoke-tests

@rosalyntan rosalyntan merged commit ecb876d into master Jun 28, 2021
@rosalyntan rosalyntan deleted the rosalyntan.gettoken branch June 28, 2021 23:14
@firebase firebase locked and limited conversation to collaborators Jul 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes Override cla size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants