Page MenuHomePhabricator
Differential D12657

[lib] Permissions for thick threads
ClosedPublic
Actions

Authored by ashoat on Jul 3 2024, 7:56 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 19, 2:26 AM
Unknown Object (File)
Fri, Apr 18, 10:13 AM
Unknown Object (File)
Fri, Apr 18, 3:49 AM
Unknown Object (File)
Thu, Apr 17, 11:40 PM
Unknown Object (File)
Thu, Apr 17, 9:56 PM
Unknown Object (File)
Thu, Apr 17, 12:55 PM
Unknown Object (File)
Tue, Apr 15, 8:45 PM
Unknown Object (File)
Wed, Apr 9, 11:43 AM
View All 100 Files
Subscribers
None

Details

Reviewers
tomek
marcin
inka
Commits
rCOMM62a52e259ee3: [lib] Permissions for thick threads
Summary
  1. Updates getRolePermissionBlobs, which is called from the migration-related function legacyUpdateRolesAndPermissions. It's important that we support thick threads there so migrations don't break on them
  2. Introduces getThickThreadRolePermissionsBlob, which is called by the above, and will be called in a later diff to get the permissions for thick threads

Depends on D12656

Test Plan

I haven't tested this stack outside of Flow. I'd like to propose that we land this so the team can iterate. I'll make sure to test all of the functionality once it's integrated into the rest of the codebase.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ashoat created this revision.Jul 3 2024, 7:56 PM
ashoat added a child revision: D12658: [lib] DMOperationSpec for CREATE_THREAD operation.
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 3 2024, 8:10 PM
Harbormaster failed remote builds in B30107: Diff 41978!
ashoat updated this revision to Diff 41984.Jul 3 2024, 8:12 PM

Rebase

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 3 2024, 8:30 PM
Harbormaster failed remote builds in B30113: Diff 41984!
ashoat updated this revision to Diff 41988.Jul 3 2024, 8:43 PM

Rebase

ashoat edited parent revisions, added: D12660: [keyserver] Update ServerThreadInfo to only cover ThinThreadType; removed: D12656: [lib] Make it possible to skip some thread/message properties.Jul 3 2024, 8:44 PM
Harbormaster completed remote builds in B30117: Diff 41988.Jul 3 2024, 9:02 PM
ashoat requested review of this revision.Jul 3 2024, 9:02 PM
inka added a comment.Jul 4 2024, 6:56 AM

I see that there is no CREATE_AND_EDIT_CHANNELS permissions, which lead me to finding a bug in our current app ENG-8710. Should we be able to create a subchannel of a thick thread?

lib/permissions/thread-permissions.js
528 ↗(On Diff #41988)

Will we still require that the users are friends before they can add each other to thick threads?

ashoat added a comment.Jul 4 2024, 10:44 AM
In D12657#358709, @inka wrote:

Should we be able to create a subchannel of a thick thread?

No, I intentionally excluded it. I think the permissions / visibility logic would get too complicated... this is something we'll limit to keyserver-hosted communities, probably indefinitely

lib/permissions/thread-permissions.js
528 ↗(On Diff #41988)

So that check doesn't happen through threadPermissions currently. But to answer your question, I think there are 3 perspectives to consider: the adder, the addee, and the peer.

We can (and should) definitely check the friendship on the adder's side.

From the perspective of a peer that receives just a "add members" operation, there is no way to validate the friendship of the adder and the addee. They would have to get a second message from the addee to confirm.

In order to check on the addee's side, we could consider splitting this into an "invite member" (adder) and "accept invite" (addee) flow.

If we do that, we would have to answer some questions:

  1. Does the pending member get to see messages while they're pending?
  2. If so, does the invite expire in some way?
  3. Do the other peers see the pending member in the member list? Are they represented in some other way?

Given the complexity, I think for now our best bet is to only do the check on the adder's side.

inka accepted this revision.Jul 5 2024, 1:49 AM
This revision is now accepted and ready to land.Jul 5 2024, 1:49 AM
Closed by commit rCOMM62a52e259ee3: [lib] Permissions for thick threads (authored by ashoat). · Explain WhyJul 8 2024, 12:47 PM
This revision was automatically updated to reflect the committed changes.
ashoat added a commit: rCOMM62a52e259ee3: [lib] Permissions for thick threads.

Revision Contents
Changeset List

PathSize
lib/
permissions/
68 lines

Diff 41978

View Options

lib/permissions/thread-permissions.js

require File.join(File.dirname(`node --print "require.resolve('expo/package.json')"`), "scripts/autolinking") require File.join(File.dirname(`node --print "require.resolve('expo/package.json')"`), "scripts/autolinking")
require File.join(File.dirname(`node --print "require.resolve('react-native/package.json')"`), "scripts/react_native_pods") require File.join(File.dirname(`node --print "require.resolve('react-native/package.json')"`), "scripts/react_native_pods")
require File.join(File.dirname(`node --print "require.resolve('@react-native-community/cli-platform-ios/package.json')"`), "native_modules") require File.join(File.dirname(`node --print "require.resolve('@react-native-community/cli-platform-ios/package.json')"`), "native_modules")
platform :ios, '12.0' platform :ios, '12.0'
require 'json' require 'json'
podfile_properties = JSON.parse(File.read('./Podfile.properties.json')) rescue {}
target 'Comm' do def common_comm_target_pods
pod 'ReactNativeKeyboardTrackingView', :path => '../../node_modules/react-native-keyboard-tracking-view'
pod 'ReactNativeKeyboardInput', :path => '../../node_modules/react-native-keyboard-input'
pod 'react-native-ffmpeg/min-lts', :podspec => '../../node_modules/react-native-ffmpeg/react-native-ffmpeg.podspec'
pod 'react-native-video/VideoCaching', :podspec => '../../node_modules/react-native-video/react-native-video.podspec'
pod 'SQLCipher-Amalgamation', :path => '../../node_modules/@commapp/sqlcipher-amalgamation' pod 'SQLCipher-Amalgamation', :path => '../../node_modules/@commapp/sqlcipher-amalgamation'
pod 'gRPC-C++', :podspec => './pod-patch/.patched/gRPC-C++/1.40.0/gRPC-C++.podspec.json' pod 'gRPC-C++', :podspec => './pod-patch/.patched/gRPC-C++/1.40.0/gRPC-C++.podspec.json'
pod 'gRPC-C++/Protobuf', :podspec => './pod-patch/.patched/gRPC-C++/1.40.0/gRPC-C++.podspec.json' pod 'gRPC-C++/Protobuf', :podspec => './pod-patch/.patched/gRPC-C++/1.40.0/gRPC-C++.podspec.json'
pod 'gRPC-Core', :podspec => './pod-patch/.patched/gRPC-Core/1.40.0/gRPC-Core.podspec.json' pod 'gRPC-Core', :podspec => './pod-patch/.patched/gRPC-Core/1.40.0/gRPC-Core.podspec.json'
pod 'Protobuf-C++', '3.15.8' pod 'Protobuf-C++', '3.15.8'
end
target 'Comm' do
pod 'ReactNativeKeyboardTrackingView', :path => '../../node_modules/react-native-keyboard-tracking-view'
pod 'ReactNativeKeyboardInput', :path => '../../node_modules/react-native-keyboard-input'
pod 'react-native-video/VideoCaching', :podspec => '../../node_modules/react-native-video/react-native-video.podspec'
pod 'react-native-ffmpeg/min-lts', :podspec => '../../node_modules/react-native-ffmpeg/react-native-ffmpeg.podspec'
common_comm_target_pods
use_expo_modules! use_expo_modules!
config = use_native_modules! config = use_native_modules!
podfile_properties = JSON.parse(File.read('./Podfile.properties.json')) rescue {}
use_react_native!( use_react_native!(
:path => config[:reactNativePath], :path => config[:reactNativePath],
# to enable hermes on iOS, change `false` to `true` and then install pods # to enable hermes on iOS, change `false` to `true` and then install pods
:hermes_enabled => podfile_properties['expo.jsEngine'] == 'hermes' :hermes_enabled => podfile_properties['expo.jsEngine'] == 'hermes'
) )
# Enables Flipper. # Enables Flipper.
# #
# Note that if you have use_frameworks! enabled, Flipper will not work and # Note that if you have use_frameworks! enabled, Flipper will not work and
# you should disable these next few lines. # you should disable these next few lines.
use_flipper!({ "Flipper-DoubleConversion" => "1.1.7" }) use_flipper!({ "Flipper-DoubleConversion" => "1.1.7" })
end
target 'NotificationService' do
common_comm_target_pods
pod 'OLMKit', :path => "../../node_modules/olm"
pod 'RCT-Folly', :podspec => "../../node_modules/react-native/third-party-podspecs/RCT-Folly.podspec"
# Lines below are only needed to compile and use Expo Secure Store in Notification Service.
# If Apple disapproves 'APPLICATION_EXTENSION_API_ONLY' flag then we can safely delete them and use
# customized Expo Secure Store that does not depend on ExpoModulesCore
podfile_properties = JSON.parse(File.read('./Podfile.properties.json')) rescue {}
use_expo_modules!
use_react_native!(
:path => "../../node_modules/react-native",
:hermes_enabled => podfile_properties['expo.jsEngine'] == 'hermes'
)
end
post_install do |installer| post_install do |installer|
ashoatUnsubmitted
Not Done
Inline Actions

Does this apply to all targets now? Are we sure we want to apply it to all targets? Is it possibly that we're inadvertedly applying it to some targets we haven't considered? (Targets that aren't the Comm app, or the notification service extension)

ashoat: Does this apply to all targets now? Are we sure we want to apply it to all targets? Is it…
marcinAuthorUnsubmitted
Done
Inline Actions

There are two changes here compared to master branch:

  1. post_install hook is moved from target 'Comm' scope to global Podfile scope
  2. some lines are added that disable APPLICATION_EXTENSION_API_ONLY flag to enable CommonCpp sources compilation in 'NotificationService' target.

Let me explain why those changes should not raise concern.
Ad. 1) According to this doc: https://www.rubydoc.info/github/CocoaPods/Core/Pod%2FPodfile%2FDSL:post_install post_install is a hook applied just before XCode project is written to disk. According to this doc: https://www.rubydoc.info/gems/cocoapods-core/0.34.1/Pod/Podfile/DSL Podfile hooks always exist in global scope. Even if this post_install hook was written inside target 'Comm' scope its behaviour would be the same as if it was in global scope. This is even more confirmed by the fact that an attempt to write multiple post_install hooks (e.g. each per target) leads to errors: https://github.com/CocoaPods/CocoaPods/issues/6172.
Ad. 2) Now that we established that we cannot have multiple post_install hooks and the one that we have should be in global scope we can admit it is impossible to use this flag anywhere else.

marcin: There are two changes here compared to master branch: 1) post_install hook is moved from target…
ashoatUnsubmitted
Not Done
Inline Actions

Thanks for the explanation!

ashoat: Thanks for the explanation!
react_native_post_install(installer) react_native_post_install(installer)
__apply_Xcode_12_5_M1_post_install_workaround(installer) __apply_Xcode_12_5_M1_post_install_workaround(installer)
# Excluding arm64 because M1 simulator isn't supported yet # Excluding arm64 because M1 simulator isn't supported yet
# Excluding 32-bit x86 because nobody uses it and it causes compilation issues # Excluding 32-bit x86 because nobody uses it and it causes compilation issues
installer.pods_project.build_configurations.each do |config| installer.pods_project.build_configurations.each do |config|
config.build_settings["EXCLUDED_ARCHS[sdk=iphonesimulator*]"] = "arm64 i386" config.build_settings["EXCLUDED_ARCHS[sdk=iphonesimulator*]"] = "arm64 i386"
ashoatUnsubmitted
Not Done
Inline Actions

It's a bit confusing to me that we exclude both of these here, but in the project.pbxproj only arm64 is mentioned. Do you know why that is?

ashoat: It's a bit confusing to me that we exclude both of these here, but in the `project.pbxproj`…
marcinAuthorUnsubmitted
Done
Inline Actions

Build settings in the XCode project refer to targets: Comm and NotificationService. This line changes build settings for pods. I did not modify this line and according to the explanation above it has alway been that we exclude both architectures for all pods. I do not know why it is that, but it is not related to any change introduced in this diff. Target 'Comm' excluded only amr64 architecture before I started to work on this issue and the very recent changes I introduced were to unify excluded architectures for both Comm and NotificationService. Would you like me to try whether we can exclude i386 arch for XCode targets as well?

marcin: Build settings in the XCode project refer to targets: Comm and NotificationService. This line…
ashoatUnsubmitted
Not Done
Inline Actions

No, that's alright – thanks for this explanation

ashoat: No, that's alright – thanks for this explanation
config.build_settings['OTHER_CPLUSPLUSFLAGS'] = '-DDONT_AUTOINSTALL_REANIMATED' config.build_settings['OTHER_CPLUSPLUSFLAGS'] = '-DDONT_AUTOINSTALL_REANIMATED'
end end
# Lines below are only needed to compile and use Expo Secure Store in Notification Service.
# If Apple disapproves 'APPLICATION_EXTENSION_API_ONLY' flag then we can safely delete them and use
# customized Expo Secure Store that does not depend on ExpoModulesCore
installer.pods_project.targets.each do |target|
target.build_configurations.each do |config|
config.build_settings['APPLICATION_EXTENSION_API_ONLY'] = 'NO'
end
end end
ashoatUnsubmitted
Not Done
Inline Actions

There's an extraneous space at the end of this line

ashoat: There's an extraneous space at the end of this line
end end