App Check App Attest initial handshake adjustments #8067
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ncooke3
approved these changes
May 12, 2021
| } | ||
|
|
||
| FIRDeviceCheckProvider(app: firebaseApp)?.getToken(completion: { token, error in | ||
| DeviceCheckProvider(app: firebaseApp)?.getToken(completion: { token, error in |
There was a problem hiding this comment.
optional: use a trailing closure
There was a problem hiding this comment.
side note... idea: WDYT about a Swift API extension to use result type here?
There was a problem hiding this comment.
Good point, I'll update it for consistency.
Note, that this particular API is not intended to be used by application developers. The method is expected to be called by the App Check core only or for testing purposes (as e.g. here). The API is public mostly because we would like to provide possibility to plug in any provider - one of predefined we have or a custom provider implemented by the app developers. Public API to get Firebase App Check token is a separate task in the backlog. We may consider using Result or/and Combine extension for it but it's out of scope for the current project.
There was a problem hiding this comment.
Note, that this particular API is not intended to be used by application developers
I see, I didn't realize. Makes sense, thanks for clarifying!
There was a problem hiding this comment.
Would we like to add a scheme to the test app that uses the SwiftUI lifecycle? If so, I could help with that in a subsequent PR (this comment is not super relevant to this thread 😅 )
There was a problem hiding this comment.
Good question. I'm not sure if App Check SDK behaves differently with app delegate and SwiftUI lifecycle because it doesn't depend on any lifecycle events yet. But feel free to add any test code you see relevant. Thank you for help!
Comment on lines
+217
to
+226
| .thenOn( | ||
| self.queue, | ||
| ^FBLPromise<NSData *> *(NSData *challengeHash) { | ||
| return [FBLPromise onQueue:self.queue | ||
| wrapObjectOrErrorCompletion:^(FBLPromiseObjectOrErrorCompletion _Nonnull handler) { | ||
| [self.appAttestService attestKey:keyID | ||
| clientDataHash:challengeHash | ||
| completionHandler:handler]; | ||
| }]; | ||
| }) |
There was a problem hiding this comment.
I'm seeing that we use .thenOn(queue, ...) and also return a [FBLPromise onQueue:queue ... ]
Question about Promises library: is specifying the target queue required in both locations for cases like this?
There was a problem hiding this comment.
Good question. If I understand it correctly, yes, it's required for both as they actually may target different queues.
|
@ncooke3 Thank you for the review. I'll merge the PR soon if you have no objections. |
ncooke3
approved these changes
May 12, 2021
maksymmalyhin
added a commit
that referenced
this pull request
Jun 1, 2021
* App A 10BC0 ttest provider: attestation sequence (#7971) * App Attest provider: attestation sequence (#761) * App Attest draft WIP * FIRAppAttestProvider initializers * ./scripts/style.sh * FIRAppAttestProvider implementation draft * Basic FIRAppAttestProviderTests and fixes * style * testGetTokenWhenAppAttestIsNotSupported * More FIRAppAttestProviderTests * Cleanup * Remove unused file * Availability annotations on DCAppAttestService category. * Guard FIRAppAttestProvider with #if TARGET_OS_IOS * Formatting * Fix SPM * app_check.yaml: Add diagnostics SPM builds * fix yaml * Fix Firebase-Package scheme bad merge * Fix typo * FIRAppAttestProvider: hide default init * FIRAppAttestKeyIDStorage: methods placeholders * Comments * Fix updated block definition * Implement app attest key ID storage (#8014) * Implement FIRAppAttestKeyIDStorage * Add FIRAppAttestKeyIDStorageTests * Review [Draft] * Style * Docs updates * Docs updates 2 * Review [Draft] 2 * Improve tests * Improve test readability * Improve test readability 2 * App Check App Attest workflow updates: initial handshake (#8032) * Handshake adjustments (WIP) * Introduce FIRAppAttestProviderState * WIP: calculate attestation state * WIP: calculate attestation state 2 * formatting * Comments and moving code around * Fix init in tests * Fix state calculation flow * Cleanup state calculation and fix tests. * Cleanup and fixes. * Comments * formatting * Fix import * Typo fixes and additional comments * FIRAppAttestInitialHandshakeResponse API * Cleanup state calculation using FBLPromiseAwait * Cleanup * style * FIRAppAttestArtifactStorage implementation and tests (#8041) * Update comments * FIRAppAttestArtifactStorage implementation and tests * Fix init * API docs * Clean up storage in tests * Comments * Disable Keychain dependent tests for SPM * Implement App Attest `getRandomChallenge` (#8033) * Initial implementation * Parse response body for challenge and stub test cases * Review [Draft] * Avoid encoding challenge again * Add tests * Revert "Avoid encoding challenge again" and add TODO This reverts commit 69eb00d. * Document tests; Add test * Tests: Add URL validation check * Review * Define Exchange AppAttest Assertion for FAC token API (#8058) * App Check App Attest: attestation request (#8059) * App Attest provider API integration WIP * update tests * Draft attestation response parsing * Attestation request draft * style * AppAttest Attestation API tests draft * Error cases tests * style * Cleanup and API docs * Merge fix * Fix OCMock imports * Fix nullability modifier * Formatting * comments * App Check App Attest initial handshake adjustments (#8067) * calculatre sha256 of random challenge for attestation * Test app adjustments * cleanup * use trailing closures in the test app * Implement API for ExchangeAppAttestAssertionRequest endpoint (#8065) * Implement assertion exchange * Tweak existing tests * Add tests * Rename JSON to better match gRPC message * Add HTTPBody helper * Review * Review 2 * Review 3 * App Check App Attest assertion flow (#8083) * App Attest assertion workflow draft * send request * assertion flow tests * style * App Check: store App Attest artifact per key ID (#8097) * Update artifact storage API and tests * Artifact storage implementation update * Save artifact for a key ID * Style * typos * App Check: prevent concurrent token requests (#8117) * App Attest multiple get token method invocation tests * Ensure a single App Attest handshake sequence at the time * FIRAppCheckTests: get token request merging tests * FIRAppCheck: Ensure a single get token operation at the time * formatting * Test new request after merged requests * Release finished operation promise * Style * Typos * typo * Request merging tests for error cases * formatting * Changelog * App Check App Attest: handle attestation rejection (#8170) * Remove/update outdated TODOs * [WIP] Attestation rejection handling draft * style * retry tests draft * reset key ID before retry * Reset attestation * test error and fixes * style * More details in the name * Some debug logging * style * Use specific codes for log messages * style * Add FIRAppAttestProvider.h the umbrella header Co-authored-by: Nick Cooke <36927374+ncooke3@users.noreply.github.com>
maksymmalyhin
added a commit
that referenced
this pull request
Jun 11, 2021
* App Attest provider: attestation sequence (#7971) * App Attest provider: attestation sequence (#761) * App Attest draft WIP * FIRAppAttestProvider initializers * ./scripts/style.sh * FIRAppAttestProvider implementation draft * Basic FIRAppAttestProviderTests and fixes * style * testGetTokenWhenAppAttestIsNotSupported * More FIRAppAttestProviderTests * Cleanup * Remove unused file * Availability annotations on DCAppAttestService category. * Guard FIRAppAttestProvider with #if TARGET_OS_IOS * Formatting * Fix SPM * app_check.yaml: Add diagnostics SPM builds * fix yaml * Fix Firebase-Package scheme bad merge * Fix typo * FIRAppAttestProvider: hide default init * FIRAppAttestKeyIDStorage: methods placeholders * Comments * Fix updated block definition * Implement app attest key ID storage (#8014) * Implement FIRAppAttestKeyIDStorage * Add FIRAppAttestKeyIDStorageTests * Review [Draft] * Style * Docs updates * Docs updates 2 * Review [Draft] 2 * Improve tests * Improve test readability * Improve test readability 2 * App Check App Attest workflow updates: initial handshake (#8032) * Handshake adjustments (WIP) * Introduce FIRAppAttestProviderState * WIP: calculate attestation state * WIP: calculate attestation state 2 * formatting * Comments and moving code around * Fix init in tests * Fix state calculation flow * Cleanup state calculation and fix tests. * Cleanup and fixes. * Comments * formatting * Fix import * Typo fixes and additional comments * FIRAppAttestInitialHandshakeResponse API * Cleanup state calculation using FBLPromiseAwait * Cleanup * style * FIRAppAttestArtifactStorage implementation and tests (#8041) * Update comments * FIRAppAttestArtifactStorage implementation and tests * Fix init * API docs * Clean up storage in tests * Comments * Disable Keychain dependent tests for SPM * Implement App Attest `getRandomChallenge` (#8033) * Initial implementation * Parse response body for challenge and stub test cases * Review [Draft] * Avoid encoding challenge again * Add tests * Revert "Avoid encoding challenge again" and add TODO This reverts commit 69eb00d. * Document tests; Add test * Tests: Add URL validation check * Review * Define Exchange AppAttest Assertion for FAC token API (#8058) * App Check App Attest: attestation request (#8059) * App Attest provider API integration WIP * update tests * Draft attestation response parsing * Attestation request draft * style * AppAttest Attestation API tests draft * Error cases tests * style * Cleanup and API docs * Merge fix * Fix OCMock imports * Fix nullability modifier * Formatting * comments * App Check App Attest initial handshake adjustments (#8067) * calculatre sha256 of random challenge for attestation * Test app adjustments * cleanup * use trailing closures in the test app * Implement API for ExchangeAppAttestAssertionRequest endpoint (#8065) * Implement assertion exchange * Tweak existing tests * Add tests * Rename JSON to better match gRPC message * Add HTTPBody helper * Review * Review 2 * Review 3 * App Check App Attest assertion flow (#8083) * App Attest assertion workflow draft * send request * assertion flow tests * style * App Check: store App Attest artifact per key ID (#8097) * Update artifact storage API and tests * Artifact storage implementation update * Save artifact for a key ID * Style * typos * App Check: prevent concurrent token requests (#8117) * App Attest multiple get token method invocation tests * Ensure a single App Attest handshake sequence at the time * FIRAppCheckTests: get token request merging tests * FIRAppCheck: Ensure a single get token operation at the time * formatting * Test new request after merged requests * Release finished operation promise * Style * Typos * typo * Request merging tests for error cases * formatting * Changelog * App Check App Attest: handle attestation rejection (#8170) * Remove/update outdated TODOs * [WIP] Attestation rejection handling draft * style * retry tests draft * reset key ID before retry * Reset attestation * test error and fixes * style * More details in the name * Some debug logging * style * Use specific codes for log messages * style * Add FIRAppAttestProvider.h the umbrella header * Add receivedAtDate property to the FAC token * Update tests to check receivedAtDate field where important * [WIP] Use FIRAppCheckTokenRefreshResult instead in the refresher API * [WIP] Fix refgresher usage * Update tests with new API, keep old logic * Update tests with new logic * WIP * WIP * Initial refresh tests and fixes. * Add #import <TargetConditionals.h> where needed. * Formatting * Cleanup and test updates * style * Comments * Cleanup and comments * Fix catalyst * Changelog * Changelog version fix * Typo * Cleanup * Typo * Imports order Co-authored-by: Nick Cooke <36927374+ncooke3@users.noreply.github.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
b/186437900
#no-changelog