[iOS] - Add ability to download and install PKPasses (Bundled) #27258

Brandon-T · 2025-01-16T00:30:12Z

Summary

Add support for PKPass bundles

Resolves brave/brave-browser#43338

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

kylehickinson · 2025-01-17T16:25:20Z

CI failing

kylehickinson · 2025-01-20T14:55:46Z

ios/brave-ios/Sources/Brave/Frontend/Sync/BraveCore/ZipImporter/ZipImporter.swift

+import Foundation
+import os.log
+
+class ZipImporter {


Why is this in Sync/BraveCore/ZipImporter? If its generic lets move it to BraveShared

kylehickinson · 2025-01-20T14:56:13Z

ios/brave-ios/Sources/Brave/Frontend/Sync/BraveCore/ZipImporter/ZipImporter.swift

+  @MainActor
+  static func unzip(path: URL) async throws -> URL {


This has no reason to be MainActor isolated

It does. It calls Brave-Core's await Unzip.unzip(nativeImportPath, toDirectory: nativeDestinationPath) and it crashes iirc, if not on the main-actor with some sequence checker.

Then that requirement should be in the core side, not here, otherwise you'd need to remember to always add MainActor isolation when unzipping using this method. If there's a sequence checker requirement then you should be using a stable task runner in the Obj-C++ side since we can't control that aspect from Swift other than forcing the code to run on main

kylehickinson · 2025-01-20T14:57:03Z

ios/brave-ios/Sources/Brave/Frontend/Sync/BraveCore/ZipImporter/ZipImporter.swift

+  static func uniqueFileName(_ filename: String, folder: URL) async throws -> URL {
+    let basePath = folder.appending(path: filename)
+    let fileExtension = basePath.pathExtension
+    let filenameWithoutExtension =
+      !fileExtension.isEmpty ? String(filename.dropLast(fileExtension.count + 1)) : filename
+
+    var proposedPath = basePath
+    var count = 0
+
+    while await AsyncFileManager.default.fileExists(atPath: proposedPath.path) {
+      count += 1
+
+      let proposedFilenameWithoutExtension = "\(filenameWithoutExtension) (\(count))"
+      proposedPath = folder.appending(path: proposedFilenameWithoutExtension)
+        .appending(path: fileExtension)
+    }
+
+    return proposedPath
+  }


This seems to be duplicated, we should move it to a shared place

kylehickinson · 2025-01-20T14:57:32Z

ios/brave-ios/Sources/BraveShared/AsyncFileManager.swift

+  /// URL where temporary files are stored.
+  public var temporaryDirectory: URL {
+    fileManager.temporaryDirectory
+  }


Can be deleted, just access FileManger.default.temporaryDirectory like normal

kylehickinson · 2025-01-20T14:59:06Z

ios/brave-ios/Sources/BraveShared/Extensions/URLExtensions.swift

+  public var fileSystemRepresentation: String? {
+    return self.withUnsafeFileSystemRepresentation { bytes -> String? in


Whats the reasoning for using the withUnsafeFileSystemRepresentation instead of just using absoluteString?

AbsoluteString doesn't strip query parameters or scheme or anything. It's just the full URL as a string representation.
fileSystemRepresentation returns a path always, with no scheme and works better with native code that requires a native file-system path.

Why wouldn't you already be dealing with a proper file path here?

Because the file path we get is from whatever the user selected in the document picker. The url is a file path but it contains the file:/// scheme and I don't want the scheme or the escaping.

kylehickinson · 2025-01-20T15:01:33Z

ios/brave-ios/Sources/Brave/Frontend/Browser/BrowserViewController/BVC+WKDownloadDelegate.swift

@@ -18,7 +18,8 @@ extension BrowserViewController: WKDownloadDelegate {
  ) async -> URL? {

    if let httpResponse = response as? HTTPURLResponse {
-      if httpResponse.mimeType != MIMEType.passbook {
+      if ![MIMEType.passbook, MIMEType.passbookBundle].contains(httpResponse.mimeType?.lowercased())


Is it actually possible for the mime-type stored in URLResponse to be not lowercase for these mimtypes?

The HTML spec says it's case-insensitive but handling of it depends on the developer. So if I send back TEXT/HTML I'm not sure if it'll be normalized to text/html when received. I'm not sure if iOS will do it.
So I lower-case it anyway.

It's seemingly normalized in Swift: https://github.com/swiftlang/swift-corelibs-foundation/blob/c466923dcaeeb058fc570f0e0cadbb04afb2b5df/Sources/FoundationNetworking/URLResponse.swift#L217

ios/brave-ios/Sources/Brave/Frontend/Browser/Helpers/OpenInHelper.swift

github-actions · 2025-01-27T18:12:05Z

Chromium major version is behind target branch (132.0.6834.83 vs 133.0.6943.27). Please rebase.

Brandon-T requested a review from stoletheminerals January 16, 2025 00:30

Brandon-T self-assigned this Jan 16, 2025

Brandon-T requested a review from a team as a code owner January 16, 2025 00:30

kylehickinson requested changes Jan 20, 2025

View reviewed changes

kylehickinson reviewed Jan 20, 2025

View reviewed changes

ios/brave-ios/Sources/Brave/Frontend/Browser/Helpers/OpenInHelper.swift Show resolved Hide resolved

Brandon-T requested a review from kylehickinson January 20, 2025 19:47

kylehickinson approved these changes Jan 20, 2025

View reviewed changes

stoletheminerals added the needs-security-review label Jan 21, 2025

github-actions bot added the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Jan 27, 2025

Brandon-T added 2 commits January 27, 2025 13:46

Add ability to download and install PKPasses (Bundled)

b0eefef

Address feedback

90a20b5

Brandon-T force-pushed the bugfix/PassKitDownloads branch from 87a5449 to 90a20b5 Compare January 27, 2025 18:47

github-actions bot removed the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Jan 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[iOS] - Add ability to download and install PKPasses (Bundled) #27258

[iOS] - Add ability to download and install PKPasses (Bundled) #27258

Brandon-T commented Jan 16, 2025

kylehickinson commented Jan 17, 2025

kylehickinson Jan 20, 2025

Brandon-T Jan 20, 2025

kylehickinson Jan 20, 2025

Brandon-T Jan 20, 2025 •

edited

Loading

kylehickinson Jan 20, 2025

Brandon-T Jan 20, 2025

kylehickinson Jan 20, 2025

Brandon-T Jan 20, 2025

kylehickinson Jan 20, 2025

kylehickinson Jan 20, 2025

Brandon-T Jan 20, 2025 •

edited

Loading

kylehickinson Jan 20, 2025

Brandon-T Jan 20, 2025 •

edited

Loading

Brandon-T Jan 20, 2025

kylehickinson Jan 20, 2025

Brandon-T Jan 20, 2025

kylehickinson Jan 20, 2025

github-actions bot commented Jan 27, 2025

		@MainActor
		static func unzip(path: URL) async throws -> URL {

		public var fileSystemRepresentation: String? {
		return self.withUnsafeFileSystemRepresentation { bytes -> String? in

[iOS] - Add ability to download and install PKPasses (Bundled) #27258

Are you sure you want to change the base?

[iOS] - Add ability to download and install PKPasses (Bundled) #27258

Conversation

Brandon-T commented Jan 16, 2025

Summary

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

kylehickinson commented Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Brandon-T Jan 20, 2025 • 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

Brandon-T Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Brandon-T Jan 20, 2025 • 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

github-actions bot commented Jan 27, 2025

Brandon-T Jan 20, 2025 •

edited

Loading

Brandon-T Jan 20, 2025 •

edited

Loading

Brandon-T Jan 20, 2025 •

edited

Loading