Skip to content
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

Implement privacy manifest aggregation #44214

Conversation

aleqsio
Copy link
Contributor

@aleqsio aleqsio commented Apr 23, 2024

Summary:

As of now, Apple does not respect privacy manifests added as cocoapods resource bundles. This forces react-native developers to manually copy .xcprivacy files content for each native dependency that accesses restricted reason APIs to the root file.

This PR adds an aggregation step that crawls through pod dependencies to collect all reasons into the root privacy info file.

Changelog:

[IOS][ADDED] – Add privacy manifest aggregation.

Test Plan:

When run on RNTester, it appends aggregated entries (while keeping existing ones) to existing .xcprivacy file without modifing .pbxproj:
image

When run on RNTester with the xcprivacy file removed from xcode beforehand, it creates a new .xcprivacy file, and adds it to Compile Bundle Resources in the same way as in the new template:
image

When run on RNTester with an empty .xcprivacy file, it appends aggregated entries from pods AND reasons for react-native core.

When run with privacy_file_aggregation_enabled: false in use_react_native, it falls back to existing behavior:
image

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Software Mansion Partner: Software Mansion Partner p: Expo Partner: Expo Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Apr 23, 2024
@facebook-github-bot
Copy link
Contributor

@philIip has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@philIip philIip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is awesome!

"NSPrivacyAccessedAPITypes" => get_core_accessed_apis
}
path = File.join(user_project.path.parent, "PrivacyInfo.xcprivacy")
Xcodeproj::Plist.write_to_path(privacy_manifest, path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feels we can reuse ensure_reference here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it depends on what we want this logic to do:

  1. Place the manifest outside of the project, and inform the user that they need to copy it manually and add their reasons? – we don't want to reference it if we don't place it in the project.

  2. Create a manifest with RN reasons inside the project (so not user_project.path.parent) and auto reference it?
    If so, perhaps we can use the same add_aggregated_privacy_manifest method and just not do aggregation to simplify the code somewhat.

Since this code path runs only if a user opts-out of the aggregation step, I'd keep it as simple as possible, do 1), and treat it as a "hey, we noticed you don't have a manifest and decided to opt-out of aggregation, here's a sample file you could use that we placed in your project root but didn't touch your actual ios project".

end

def self.get_application_target(user_project)
return user_project.targets.first{ |t| t.symbol_type == :application }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we iterate through all of the targets instead? will there always be one target with a symbol_type of symbol?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh I'm not sure, we could either only do it for the first target or do all the following steps for all matching targets, potentially creating multiple PrivacyInfo files and linking to each corresponding target.

@cipolleschi – do you think this check is enough?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that creating a privacy manifest for each 'application' target is the most correct approach.
Users might have a single project for iOS, MacOS, VisionOS and AppleTV apps... and in some very weird cases, even multiple iOS apps in the same project, to share most code, like a monorepo setup.

I had to work in a similar scenario in the past, tbh. But I think it is an edge case for the React Native ecosystem.

Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments, overall looks very good!
Many thanks for working on this!


def self.get_privacyinfo_file_path(target, user_project)
# We try to find a file we know exists in the project to get the path to the main group directory
app_delegate_path = target.source_build_phase.files_references.find { |file_ref| file_ref.name == "Info.plist" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
app_delegate_path = target.source_build_phase.files_references.find { |file_ref| file_ref.name == "Info.plist" }
info_plist_path = target.source_build_phase.files_references.find { |file_ref| file_ref.name == "Info.plist" }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also changed it to read it from the user_project files, since info.plist does not end up in source_build_phase after all.

end

def self.get_application_target(user_project)
return user_project.targets.first{ |t| t.symbol_type == :application }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that creating a privacy manifest for each 'application' target is the most correct approach.
Users might have a single project for iOS, MacOS, VisionOS and AppleTV apps... and in some very weird cases, even multiple iOS apps in the same project, to share most code, like a monorepo setup.

I had to work in a similar scenario in the past, tbh. But I think it is an edge case for the React Native ecosystem.

packages/react-native/scripts/react_native_pods.rb Outdated Show resolved Hide resolved
@aleqsio
Copy link
Contributor Author

aleqsio commented Apr 26, 2024

Now I'm looping over all application targets to add references – didn't really test it too much, but it seems pretty self-contained.

Generating the file and adding to one application target still works after recent changes
image
image

@aleqsio aleqsio mentioned this pull request Apr 29, 2024
3 tasks
@facebook-github-bot
Copy link
Contributor

@philIip has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label May 1, 2024
@facebook-github-bot
Copy link
Contributor

@philIip merged this pull request in 61f584c.

Copy link

github-actions bot commented May 1, 2024

This pull request was successfully merged by @aleqsio in 61f584c.

When will my fix make it into a release? | How to file a pick request?

cipolleschi pushed a commit that referenced this pull request May 2, 2024
Summary:
As of now, Apple does not respect privacy manifests added as cocoapods resource bundles. This forces react-native developers to manually copy `.xcprivacy` files content for each native dependency that accesses restricted reason APIs to the root file.

This PR adds an aggregation step that crawls through pod dependencies to collect all reasons into the root privacy info file.

[IOS][ADDED] – Add privacy manifest aggregation.

Pull Request resolved: #44214

Test Plan:
When run on RNTester, it appends aggregated entries (while keeping existing ones) to existing .xcprivacy file without modifing .pbxproj:
![image](https://github.com/facebook/react-native/assets/5597580/1d07a07d-bbec-4266-a599-a8d629078971)

When run on RNTester with the xcprivacy file removed from xcode beforehand, it creates a new .xcprivacy file, and adds it to Compile Bundle Resources in the same way as in the new template:
![image](https://github.com/facebook/react-native/assets/5597580/f80a3b4e-e41a-4906-8e2f-06cca0bc225a)

When run on RNTester with an empty .xcprivacy file, it appends aggregated entries from pods AND reasons for react-native core.

When run with `privacy_file_aggregation_enabled: false` in `use_react_native`, it falls back to existing behavior:
![image](https://github.com/facebook/react-native/assets/5597580/4519bba1-c80e-4cd0-b19c-bbbebfa8493b)

Reviewed By: cipolleschi

Differential Revision: D56481045

Pulled By: philIip

fbshipit-source-id: 1841bad821511c734d0cc0fcff5065ed92af76d8
reference_exists = target.resources_build_phase.files_references.any? { |file_ref| file_ref.path.end_with? "PrivacyInfo.xcprivacy" }
unless reference_exists
# We try to find the main group, but if it doesn't exist, we default to adding the file to the project root – both work
file_root = user_project.root_object.main_group.children.first { |group| group.name == target.name } || user_project
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aleqsio I'm trying to backport this to 0.72 in advance of reactwg/react-native-releases#279 being done. I think this line might be naive/incorrect.

I believe I've integrated the changes into 0.72.14 correctly but when I try and pod install I get:

image

I've used pry to debug what's going on. It looks like the .first should be a .find (first only returns the first item in a collection) and, in my case .first is not a group, it's an xcconfig file:

image

Equivalent project structure view in Xcode:

image

While debugging, I also discovered that the groups have nil names, I think we want to match against path?

I've changed the line as follows:

file_root = user_project.root_object.main_group.children.find { |item| item.class == Xcodeproj::Project::Object::PBXGroup && item.path == target.name } || user_project

and it now integrates, finding the group name that matches my project name and adding the privacy manifest file to there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolutely correct, thanks for flagging 😍

The reason why it passed through my testing was that the privacymanifest file was the first in the array in rntester, AND the code block got swallowed without error – joys of ruby programming I guess.

facebook-github-bot pushed a commit that referenced this pull request May 2, 2024
Summary:
As pointed out by liamjones here:
#44214 (comment)

The original PR did introduce a bug in the `find/first` check, but in my testing, we do need to look at `group.name`, so let's make sure we check both.

This also makes it play nice with an existing file even if it is added to a different directory, by appending to it instead of forcing it to exist in the main group.

## Changelog:

[IOS] [FIXED] - Fix privacy aggregation

Pull Request resolved: #44390

Test Plan: Tested on rn-tester

Reviewed By: cipolleschi

Differential Revision: D56893594

Pulled By: philIip

fbshipit-source-id: b92589bc2bed9d07e9af20c56a8b9f6c61d864f0
kosmydel pushed a commit to kosmydel/react-native that referenced this pull request May 6, 2024
Summary:
As of now, Apple does not respect privacy manifests added as cocoapods resource bundles. This forces react-native developers to manually copy `.xcprivacy` files content for each native dependency that accesses restricted reason APIs to the root file.

This PR adds an aggregation step that crawls through pod dependencies to collect all reasons into the root privacy info file.

## Changelog:

[IOS][ADDED] – Add privacy manifest aggregation.

Pull Request resolved: facebook#44214

Test Plan:
When run on RNTester, it appends aggregated entries (while keeping existing ones) to existing .xcprivacy file without modifing .pbxproj:
![image](https://github.com/facebook/react-native/assets/5597580/1d07a07d-bbec-4266-a599-a8d629078971)

When run on RNTester with the xcprivacy file removed from xcode beforehand, it creates a new .xcprivacy file, and adds it to Compile Bundle Resources in the same way as in the new template:
![image](https://github.com/facebook/react-native/assets/5597580/f80a3b4e-e41a-4906-8e2f-06cca0bc225a)

When run on RNTester with an empty .xcprivacy file, it appends aggregated entries from pods AND reasons for react-native core.

When run with `privacy_file_aggregation_enabled: false` in `use_react_native`, it falls back to existing behavior:
![image](https://github.com/facebook/react-native/assets/5597580/4519bba1-c80e-4cd0-b19c-bbbebfa8493b)

Reviewed By: cipolleschi

Differential Revision: D56481045

Pulled By: philIip

fbshipit-source-id: 1841bad821511c734d0cc0fcff5065ed92af76d8
kosmydel pushed a commit to kosmydel/react-native that referenced this pull request May 6, 2024
Summary:
As pointed out by liamjones here:
facebook#44214 (comment)

The original PR did introduce a bug in the `find/first` check, but in my testing, we do need to look at `group.name`, so let's make sure we check both.

This also makes it play nice with an existing file even if it is added to a different directory, by appending to it instead of forcing it to exist in the main group.

## Changelog:

[IOS] [FIXED] - Fix privacy aggregation

Pull Request resolved: facebook#44390

Test Plan: Tested on rn-tester

Reviewed By: cipolleschi

Differential Revision: D56893594

Pulled By: philIip

fbshipit-source-id: b92589bc2bed9d07e9af20c56a8b9f6c61d864f0
@matinzd
Copy link
Contributor

matinzd commented May 9, 2024

If a pod or multiple pods are deleted, shouldn't we make sure xcprivacy updates accordingly?

@liamjones
Copy link
Contributor

@matinzd I think the problem there would be knowing what the user added to the root manifest themselves for their own code (and thus should remain) vs what was added only for a 3rd-party dependency (and thus should be removed).

The way we're handling it at our company is that we don't put our codes into the root manifest directly. We instead have a minimal app-specific cocoapod dependency which contains our reason codes and we blank the root manifest at the beginning of a pod install so that RN aggregates our manifest alongside the 3rd-party dependencies manifests.

@aleqsio
Copy link
Contributor Author

aleqsio commented May 9, 2024

@matinzd @liamjones Blanking the file is a good approach, or alternatively you can remove the privacy file manually when removing some pods – I assumed it's better to include more reasons than risk submit rejection, so this tool doesn't clear out entries from the privacy file by default.

I also considered marking entries or saving a shadow file to track removed reasons or a multitude of other approaches, but they all added complexity that seemed more problem that it's worth.

alfonsocj pushed a commit that referenced this pull request Jun 10, 2024
Summary:
As of now, Apple does not respect privacy manifests added as cocoapods resource bundles. This forces react-native developers to manually copy `.xcprivacy` files content for each native dependency that accesses restricted reason APIs to the root file.

This PR adds an aggregation step that crawls through pod dependencies to collect all reasons into the root privacy info file.

[IOS][ADDED] – Add privacy manifest aggregation.

Pull Request resolved: #44214

Test Plan:
When run on RNTester, it appends aggregated entries (while keeping existing ones) to existing .xcprivacy file without modifing .pbxproj:
![image](https://github.com/facebook/react-native/assets/5597580/1d07a07d-bbec-4266-a599-a8d629078971)

When run on RNTester with the xcprivacy file removed from xcode beforehand, it creates a new .xcprivacy file, and adds it to Compile Bundle Resources in the same way as in the new template:
![image](https://github.com/facebook/react-native/assets/5597580/f80a3b4e-e41a-4906-8e2f-06cca0bc225a)

When run on RNTester with an empty .xcprivacy file, it appends aggregated entries from pods AND reasons for react-native core.

When run with `privacy_file_aggregation_enabled: false` in `use_react_native`, it falls back to existing behavior:
![image](https://github.com/facebook/react-native/assets/5597580/4519bba1-c80e-4cd0-b19c-bbbebfa8493b)

Reviewed By: cipolleschi

Differential Revision: D56481045

Pulled By: philIip

fbshipit-source-id: 1841bad821511c734d0cc0fcff5065ed92af76d8
kosmydel pushed a commit to kosmydel/react-native that referenced this pull request Jun 11, 2024
Summary:
As of now, Apple does not respect privacy manifests added as cocoapods resource bundles. This forces react-native developers to manually copy `.xcprivacy` files content for each native dependency that accesses restricted reason APIs to the root file.

This PR adds an aggregation step that crawls through pod dependencies to collect all reasons into the root privacy info file.

## Changelog:

[IOS][ADDED] – Add privacy manifest aggregation.

Pull Request resolved: facebook#44214

Test Plan:
When run on RNTester, it appends aggregated entries (while keeping existing ones) to existing .xcprivacy file without modifing .pbxproj:
![image](https://github.com/facebook/react-native/assets/5597580/1d07a07d-bbec-4266-a599-a8d629078971)

When run on RNTester with the xcprivacy file removed from xcode beforehand, it creates a new .xcprivacy file, and adds it to Compile Bundle Resources in the same way as in the new template:
![image](https://github.com/facebook/react-native/assets/5597580/f80a3b4e-e41a-4906-8e2f-06cca0bc225a)

When run on RNTester with an empty .xcprivacy file, it appends aggregated entries from pods AND reasons for react-native core.

When run with `privacy_file_aggregation_enabled: false` in `use_react_native`, it falls back to existing behavior:
![image](https://github.com/facebook/react-native/assets/5597580/4519bba1-c80e-4cd0-b19c-bbbebfa8493b)

Reviewed By: cipolleschi

Differential Revision: D56481045

Pulled By: philIip

fbshipit-source-id: 1841bad821511c734d0cc0fcff5065ed92af76d8
kosmydel pushed a commit to kosmydel/react-native that referenced this pull request Jun 11, 2024
Summary:
As pointed out by liamjones here:
facebook#44214 (comment)

The original PR did introduce a bug in the `find/first` check, but in my testing, we do need to look at `group.name`, so let's make sure we check both.

This also makes it play nice with an existing file even if it is added to a different directory, by appending to it instead of forcing it to exist in the main group.

## Changelog:

[IOS] [FIXED] - Fix privacy aggregation

Pull Request resolved: facebook#44390

Test Plan: Tested on rn-tester

Reviewed By: cipolleschi

Differential Revision: D56893594

Pulled By: philIip

fbshipit-source-id: b92589bc2bed9d07e9af20c56a8b9f6c61d864f0
gabrieldonadel pushed a commit that referenced this pull request Jul 1, 2024
Summary:
As of now, Apple does not respect privacy manifests added as cocoapods resource bundles. This forces react-native developers to manually copy `.xcprivacy` files content for each native dependency that accesses restricted reason APIs to the root file.

This PR adds an aggregation step that crawls through pod dependencies to collect all reasons into the root privacy info file.

[IOS][ADDED] – Add privacy manifest aggregation.

Pull Request resolved: #44214

Test Plan:
When run on RNTester, it appends aggregated entries (while keeping existing ones) to existing .xcprivacy file without modifing .pbxproj:
![image](https://github.com/facebook/react-native/assets/5597580/1d07a07d-bbec-4266-a599-a8d629078971)

When run on RNTester with the xcprivacy file removed from xcode beforehand, it creates a new .xcprivacy file, and adds it to Compile Bundle Resources in the same way as in the new template:
![image](https://github.com/facebook/react-native/assets/5597580/f80a3b4e-e41a-4906-8e2f-06cca0bc225a)

When run on RNTester with an empty .xcprivacy file, it appends aggregated entries from pods AND reasons for react-native core.

When run with `privacy_file_aggregation_enabled: false` in `use_react_native`, it falls back to existing behavior:
![image](https://github.com/facebook/react-native/assets/5597580/4519bba1-c80e-4cd0-b19c-bbbebfa8493b)

Reviewed By: cipolleschi

Differential Revision: D56481045

Pulled By: philIip

fbshipit-source-id: 1841bad821511c734d0cc0fcff5065ed92af76d8
gabrieldonadel pushed a commit that referenced this pull request Jul 1, 2024
Summary:
As pointed out by liamjones here:
#44214 (comment)

The original PR did introduce a bug in the `find/first` check, but in my testing, we do need to look at `group.name`, so let's make sure we check both.

This also makes it play nice with an existing file even if it is added to a different directory, by appending to it instead of forcing it to exist in the main group.

## Changelog:

[IOS] [FIXED] - Fix privacy aggregation

Pull Request resolved: #44390

Test Plan: Tested on rn-tester

Reviewed By: cipolleschi

Differential Revision: D56893594

Pulled By: philIip

fbshipit-source-id: b92589bc2bed9d07e9af20c56a8b9f6c61d864f0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Expo Partner: Expo p: Software Mansion Partner: Software Mansion Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants