Page MenuHomePhabricator

Enable App Groups in Comm and Notification Service
ClosedPublic

Authored by marcin on May 10 2022, 4:39 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 17, 4:19 PM
Unknown Object (File)
Tue, Dec 17, 4:19 PM
Unknown Object (File)
Tue, Dec 17, 4:19 PM
Unknown Object (File)
Tue, Dec 17, 4:19 PM
Unknown Object (File)
Tue, Dec 17, 4:19 PM
Unknown Object (File)
Tue, Dec 17, 4:19 PM
Unknown Object (File)
Tue, Dec 17, 4:19 PM
Unknown Object (File)
Tue, Dec 17, 4:19 PM

Details

Summary

This diffs enables usage of App Groups in NotificationService and Comm so that they can securely share files - e.g. SQlite Database

Test Plan

Not testing plan for this diff. Its child will introduce functions that use App groups to share SQLite database between NotificationService and Comm that will have testing plan.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

native/ios/NotificationService/NotificationService.entitlements
7 ↗(On Diff #12494)

Can you link any resources for how to determine this name? It seems a bit strange that the TLD is group here

native/ios/NotificationService/NotificationService.entitlements
7 ↗(On Diff #12494)
native/ios/NotificationService/NotificationService.entitlements
7 ↗(On Diff #12494)

The docs mention a "group name", and you've opted for app.comm there which is our bundle identifier. Is that a standard practice? Can you link some docs to show that it is a standard practice?

tomek added a reviewer: ashoat.

This seems fine, but adding @ashoat so that we can clarify the group name

ashoat requested changes to this revision.May 11 2022, 6:03 PM

See question above

This revision now requires changes to proceed.May 11 2022, 6:03 PM
marcin requested review of this revision.EditedMay 16 2022, 3:26 AM

I found this piece of apple docs: https://developer.apple.com/documentation/bundleresources/entitlements/com_apple_security_application-groups?language=objc, which suggests that developers are not restricted to any particular group name scheme. However I have also found docs for two iOS 3rd party app extensions:
https://doc.batch.com/ios/advanced/app-groups and https://documentation.onesignal.com/docs/ios-sdk-app-groups-setup that use the following schema to share data with the main app via app groups: group.<bundle id>.<3rd party extension name>. Therefore I would assume that it is a good practice to name app group like group.<bundle id>.<extension name> if it is meant to share data between app and app extension. Based on those findings I could change group name from group.app.comm to group.app.comm.notification_service, but on the other hand the reason we are creating this app group is that we want notification service to share SQLite database with the main app. If we ever use another app extension that would need to access sqlite database it should also be in this group so group.app.comm.notification_service will no longer be valid. Renaming this app group would be troublesome as we would need to implement another SQLite migration. In conclusion I suggest that we name this app group with something like: group.app.comm.sqlite_dependent. @ashoat @palys-swm let me know what you think.

In D3992#113648, @marcinwasowicz wrote:

I found this piece of apple docs: https://developer.apple.com/documentation/bundleresources/entitlements/com_apple_security_application-groups?language=objc, which suggests that developers are not restricted to any particular group name scheme. However I have also found docs for two iOS 3rd party app extensions:
https://doc.batch.com/ios/advanced/app-groups and https://documentation.onesignal.com/docs/ios-sdk-app-groups-setup that use the following schema to share data with the main app via app groups: group.<bundle id>.<3rd party extension name>. Therefore I would assume that it is a good practice to name app group like group.<bundle id>.<extension name> if it is meant to share data between app and app extension. Based on those findings I could change group name from group.app.comm to group.app.comm.notification_service, but on the other hand the reason we are creating this app group is that we want notification service to share SQLite database with the main app. If we ever use another app extension that would need to access sqlite database it should also be in this group so group.app.comm.notification_service will no longer be valid. Renaming this app group would be troublesome as we would need to implement another SQLite migration. In conclusion I suggest that we name this app group with something like: group.app.comm.sqlite_dependent. @ashoat @palys-swm let me know what you think.

Great research!

Do you know if one app can be a part of multiple app groups? E.g. our main app and notification service extension would be one group, our main app and other extension the second, etc. If that's the case we have greater flexibility in choosing the name. Regardless, I don't have a strong opinion but I don't see a good reason for not choosing group.app.comm. It looks like any other app we will add, could also require access to the same db, so it's quite universal. Having a name that specifies a dependency sounds risky, because if there are more common dependencies, we would need to update the name. From the examples you provided, it looks like the groups are used for third-party extensions - if we ever introduce one (unlikely), we will create a separate group for it.

Thanks for that research, @marcinwasowicz! Agree with @palys-swm's reasoning – I think group.app.comm is probably fine, but am curious if it's possible for an app to be part of multiple app groups.

Can you clarify why you planned changes? I was going to accept the diff, but I'm not sure if I should now.

Answering two questions here:

  1. https://developer.apple.com/documentation/xcode/configuring-app-groups?changes=latest_minor, This doc explicitly state: "Apps can belong to more that one app group".
  2. I hit "plan changes by accident". I intended to do for diff D3370 after rebasing it and made silly mistake since it begins with the same word "Enable".
This revision is now accepted and ready to land.May 17 2022, 12:39 PM
This revision was landed with ongoing or failed builds.May 24 2022, 1:47 AM
This revision was automatically updated to reflect the committed changes.