AppIntegrity Feature #232

jeyanthanperiyasamy · 2023-09-18T16:39:58Z

JIRA Ticket

Please, link jira ticket here.

Description

Briefly describe the change and any information that would help speedup the review and testing process.

Definition of Done Checklist:

george-bafaloukas-forgerock · 2023-09-25T16:22:54Z

FRAuth/FRAuth.xcodeproj/xcshareddata/xcschemes/FRAuth.xcscheme

@@ -57,6 +57,11 @@
               BlueprintName = "FRAuthTests"
               ReferencedContainer = "container:FRAuth.xcodeproj">
            </BuildableReference>
+            <SkippedTests>
+               <Test
+                  Identifier = "AA_07_AppIntegrityTest/test_01_test_app_integrity()">


Why are we skipping this? Should that be committed

The tests are written and the classes are committed, i just disabled tests because it will only work in local box for now and feature is not available in AM , so when the AM is ready we will add this test back. otherwise it will fail the tests in bitbar when this story merge to dev branch

george-bafaloukas-forgerock · 2023-09-25T16:24:35Z

FRAuth/FRAuth/AppIntegrity/FRAppAttestDomainModal.swift

+         case .featureUnsupported:
+            return "Unsupported"
+        default:
+            return "ClientDeviceErrors"


I suggest we centralize the strings for errors or outcomes. Either in a utility class (we might have one already) or at least top of the file, on a let

taken care , i abstracted all the errors in to his own enum.

george-bafaloukas-forgerock · 2023-09-25T16:25:43Z

FRAuth/FRAuth/Callbacks/FRAppIntegrityCallback.swift

+        }
+        self.assertToken = assertKey
+
+        guard let clientData = inputNames.filter({ $0.contains("IDToken1clientData") }).first else {


Make the keys lets or a an enum

witrisna · 2023-09-27T23:00:49Z

FRAuth/FRAuth/AppIntegrity/FRAppAttestDomainModal.swift

+
+/// Protocol to override attestation
+@available(iOS 14.0, *)
+public protocol FRAppAttestation {


Should we consider FRAppAttestation , FRAppAttestDomainModal, FRAppAttestService as internal class?
Do we need developer to have access of those interfaces?

@witrisna i think the constructor injection in once of class made all the cascading class to public, i have provided the fix, this will be internal . if you remove public its default to internal in swift . Good Catch

rodrigoareis

Overall changes looks good. Minor comments added on top what was already spotted.

rodrigoareis · 2023-09-28T00:43:06Z

FRAuth/FRAuth/AppIntegrity/FRAppAttestServiceImpl.swift

+    ///   - keyIdentifier: The identifier you received when generating a cryptographic key by calling the generateKey(completionHandler:) method.
+    ///   - clientDataHash: A SHA256 hash of a unique, single-use data block that represents the client data to be signed with the attested private key.
+    /// - Returns: A data structure that you send to your server for processing OR A DCError instance that indicates the reason for failure, or nil on success.
+    public func generateAssertion(keyIdentifier: String, clientDataHash: Data) async throws -> Data {


Add Throws to doc

rodrigoareis · 2023-09-28T00:43:28Z

FRAuth/FRAuth/AppIntegrity/FRAppAttestServiceImpl.swift

+    }
+
+    /// Creates a new cryptographic key for use with the App Attest service.
+    public func generateKey() async throws -> String {


Add Throws and Returns to doc

rodrigoareis · 2023-09-28T00:43:46Z

FRAuth/FRAuth/AppIntegrity/FRAppAttestServiceImpl.swift

+    }
+
+    /// A Boolean value that indicates whether a particular device provides the App Attest service.
+    public func isSupported() -> Bool {


Add Returns to doc

vahancouver

Left some comments to address

Overall looks great. Great amount of test coverage

vahancouver · 2023-09-28T14:37:33Z

FRAuth/FRAuth/AppIntegrity/FRAppAttestDomainModal.swift

+    ///
+    /// - Parameter service: FRAppAttestService to connect AppAttestation server
+    /// - Parameter bundleIdentifier: BundleId of the application
+    /// - encoder encoder: Encoder to endode the Data


Should read "- Parameter encoder" and there is a type in encode word

vahancouver · 2023-09-28T14:51:39Z

FRAuth/FRAuth/AppIntegrity/FRAppAttestDomainModal.swift

+    /// Handle attestation and assertion
+    /// - Parameter challenge: Challenge Received from server
+    /// - Returns: FRAppIntegrityKeys for attestation and assertion
+    public func attest(challenge: String) async throws -> FRAppIntegrityKeys {


Add throws to docs

vahancouver · 2023-09-28T15:17:29Z

FRAuth/FRAuth/AppIntegrity/FRAppAttestDomainModal.swift

+        default:
+            return FRAppIntegrityClientError.clientDeviceErrors.rawValue
+        }
+    }


Instead of this map Error method, I suggest adding a property in FRDeviceCheckAPIFailure like this code below. I think it's cleaner especially when using it

var clientError: String {
switch self {
case .featureUnsupported:
return FRAppIntegrityClientError.unSupported.rawValue
default:
return FRAppIntegrityClientError.clientDeviceErrors.rawValue
}
}

perfect, i like this one. lets do this @vahancouver

vahancouver · 2023-09-28T15:22:34Z

FRAuth/FRAuth/AppIntegrity/FRAppAttestDomainModal.swift

+@available(iOS 14.0, *)
+public protocol FRAppAttestation {
+    func attest(challenge: String) async throws -> FRAppIntegrityKeys
+}


Should we add doumentation for protocol methods as well?

i am about to modify this and make as a internal protocol . i am happy to add it if thats a team standard, but i like to add docs for any public facing methods

vahancouver · 2023-09-28T15:22:46Z

FRAuth/FRAuth/AppIntegrity/FRAppAttestServiceImpl.swift

+    func attest(keyIdentifier: String, clientDataHash: Data) async throws -> Data
+    func generateAssertion(keyIdentifier: String, clientDataHash: Data) async throws -> Data
+    func isSupported() -> Bool
+}


Should we add doumentation for protocol methods as well?

vahancouver · 2023-09-28T15:26:42Z

FRAuth/FRAuth/Callbacks/FRAppIntegrityCallback.swift

+    public private(set) var assertToken: String
+
+    /// keys received from AM
+    private enum keys: String {


Enum name should be uppercase

i will remove this keys enum and add in to the CBConstants

vahancouver · 2023-09-28T15:32:37Z

FRAuth/FRAuth/Callbacks/FRAppIntegrityCallback.swift

+        case clientError = "IDToken1clientError"
+        case keyId = "IDToken1keyId"
+        case clientData = "IDToken1clientData"
+    }


Also in other callbacks we never add local Keys enum. we try to reuse CBConstants, and if anything is missing, let's add there

vahancouver · 2023-09-28T15:38:37Z

FRAuth/FRAuth/Callbacks/FRAppIntegrityCallback.swift

+    /// - Parameter attestation: Optional Protocol for providing a ``FRAppAttestation`` to implement own attestation
+    @available(iOS 14.0, *)
+    public func attest(attestation: FRAppAttestation
+                       = FRAppAttestDomainModal()) async throws {


Add throws to docs

jeyanthanperiyasamy · 2023-10-02T14:08:11Z

all the comments where addressed , please review one more time and we can call this complete @vahancouver @george-bafaloukas-forgerock @rodrigoareis @witrisna .

vahancouver

Changes look good. All comments addressed

jeyanthanperiyasamy force-pushed the SDKS-2630 branch 7 times, most recently from d6986d4 to 21bbdcf Compare September 25, 2023 06:18

jeyanthanperiyasamy requested review from spetrov, rodrigoareis, vahancouver, george-bafaloukas-forgerock and witrisna September 25, 2023 06:20

george-bafaloukas-forgerock requested changes Sep 25, 2023

View reviewed changes

jeyanthanperiyasamy force-pushed the SDKS-2630 branch 4 times, most recently from 47663b5 to 6d70c8f Compare September 27, 2023 22:40

witrisna reviewed Sep 27, 2023

View reviewed changes

rodrigoareis reviewed Sep 28, 2023

View reviewed changes

vahancouver requested changes Sep 28, 2023

View reviewed changes

AppIntegrity feature

17f4e99

jeyanthanperiyasamy force-pushed the SDKS-2630 branch from 6d70c8f to 17f4e99 Compare September 29, 2023 00:59

vahancouver approved these changes Oct 3, 2023

View reviewed changes

jeyanthanperiyasamy merged commit 17f4e99 into develop Oct 25, 2023
5 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AppIntegrity Feature #232

AppIntegrity Feature #232

jeyanthanperiyasamy commented Sep 18, 2023

george-bafaloukas-forgerock Sep 25, 2023

jeyanthanperiyasamy Sep 25, 2023 •

edited

Loading

george-bafaloukas-forgerock Sep 25, 2023

jeyanthanperiyasamy Sep 27, 2023

george-bafaloukas-forgerock Sep 25, 2023

jeyanthanperiyasamy Sep 27, 2023

witrisna Sep 27, 2023

jeyanthanperiyasamy Sep 28, 2023

rodrigoareis left a comment

rodrigoareis Sep 28, 2023

rodrigoareis Sep 28, 2023

rodrigoareis Sep 28, 2023

vahancouver left a comment

vahancouver Sep 28, 2023

vahancouver Sep 28, 2023

vahancouver Sep 28, 2023

jeyanthanperiyasamy Sep 28, 2023 •

edited

Loading

vahancouver Sep 28, 2023

jeyanthanperiyasamy Sep 28, 2023

vahancouver Sep 28, 2023

vahancouver Sep 28, 2023

jeyanthanperiyasamy Sep 28, 2023

vahancouver Sep 28, 2023

vahancouver Sep 28, 2023

jeyanthanperiyasamy commented Oct 2, 2023 •

edited

Loading

vahancouver left a comment

AppIntegrity Feature #232

AppIntegrity Feature #232

Conversation

jeyanthanperiyasamy commented Sep 18, 2023

JIRA Ticket

Description

Definition of Done Checklist:

Choose a reason for hiding this comment

jeyanthanperiyasamy Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigoareis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vahancouver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeyanthanperiyasamy Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeyanthanperiyasamy commented Oct 2, 2023 • edited Loading

vahancouver left a comment

Choose a reason for hiding this comment

jeyanthanperiyasamy Sep 25, 2023 •

edited

Loading

jeyanthanperiyasamy Sep 28, 2023 •

edited

Loading

jeyanthanperiyasamy commented Oct 2, 2023 •

edited

Loading