Allow registering as a linked device#157
Conversation
|
This is very interesting, but it's a poorly documented. I need your help to review it. Also there are changes in the code, unrelated to device linking, that undo recent commits already in main. What version of Molly are the changes based on? |
|
It was originally built on top of dba9f3e then rebased to 1225c56. All the changes in the libsignal/service folder are 3-way merged between this repo, the original signal repo, and: There are a few files that might initially look like they are reverted/unrelated changes. They are not. These are: The changes in these files is to adjust for an API change in libsignal/service which allows group V1 and group V2 to co-exist in the API. The class There is another change in the API, that I ended up applying in the opposite direction (i.e. reverting the API change). That is changing the definitions of device id parameters from being sometimes Once you set aside these changes, what's left (i.e. The parts I actually wrote) should be far easier to understand. I've made comments on the significantly hacky parts. If you have any specific questions about any of the other parts, I'll do my best to answer. |
Turasa/libsignal-service-java has dozens of commits and bugfixes that are not related to this pull-request. It is not wise to automatically merge them into Molly. This is a critical component under heavy development by Signal. Also, the latest commit is from Nov 7th and Molly's libsignal is newer. There's no way I can review this and maintain it later. You would need a clean, manually curated merge, that includes only the minimal set of changes required to register as a linked device. |
3cb56dd to
087ffb7
Compare
|
Okay, I made a best effort to reduce the changes to only what is absolutely necessary for device linking. Linking and, at the very least, sending a note to self, is tested and still working. It is now far fewer changes, and should be easier to read. |
087ffb7 to
dc108dc
Compare
|
Fixed lint errors |
|
Thank you. Now it's much neater. There are still some leftovers, but it's not a problem. I'd like to try this out right away before discussing the implementation details. We have a dedicated testing group and they will love to try this out. I can build one testing APK with your code merged. Would you consider joining our testing channel in Matrix? I'm in that channel too. |
|
APK ready for testing: https://github.com/mollyim/mollyim-insider-android/releases/tag/v6.3.6-1%2Blink-device |
dc108dc to
d16bcd4
Compare
|
Added possible fix for contact and group sync (requires testing). Also, made it so the pin setting popup doesn't appear, since it's impossible to set pin from the linked device (It instead gets the master key directly from the primary). |
d16bcd4 to
292807f
Compare
|
Trying to interact with the testers on the testing channel, but it seems like they're not seeing my messages. Must be a Matrix bug. So I'll just point out here that one thing in particular that I want tested, is sync of blocked contacts. This should absolutely work, even with the previous version. And if it's not working, then it means that there's a serious issue that needs to be investigated. In particular, I want the following process tested:
I only have a limited ability to test this alone (I don't have a bunch of Android phones lying around), so input from testers is valuable. |
Great. I'm building a new testing APK.
I haven't received any message from you since you joined the room. May rejoining fix it? Also Element Web seems to work fine most of the time.
You can try emulators. That's what I use for testing. If you need extra phone numbers for registering in production servers, please let me know. Up to now, the testers have identified the following issues and improvements:
|
|
Debug log for the extra message issue: https://debuglogs.app/android/6.3.6/5f0a809d47011792659b548e5994cd9a8c4414c7fa9928551411cdd880c2007b Message-ID: 1671545932600 |
|
The Matrix issue probably stems from the fact that I'm not using a matrix.org user. There's probably a bug in federation. That's my best guess, anyway. The empty sync job is a bit difficult to debug. First, because it appears random. Second, because the log contains multiple attempts, calls, even a re-register as primary. Without at least a timestamp narrowing it down, I can't even tell where in the log I need to look. The log does give some interesting messages that can be investigated regardless, though. I can see the part where it fails reading group and PNI sync messages. The prior is a bit unexpected, because those should only transmit for V1 groups. I'm also seeing indication that contact and key sync messages are being read okay. That's a good sign, and I'd like to know if, combined with the above fix to storage, it allows a full sync. Registering a second secondary from primary should, by all means, work. My own device is a third secondary, since I have two desktop clients attached. My guess here is that perhaps the tester did multiple link attempts, but did not delete previous linked devices from the primary. This could have caused it to reach the maximum of 3 linked devices. If that's not the case, I'm going to need the error timestamp in the log. I'll look into the others since they are simple UI changes. I assume the reason to remove account deletion from a linked device is because it doesn't work, and can only be done from primary. Besides account deletion, the effects of device deletion need to be investigated as well. As in, in the device list if you tap on a device, it offers the option to delete it. If this doesn't work from a linked device, the UI option should be removed. If it does work, then the effects of doing a self-delete need to be investigated, in case it outright crashes the client or something. |
do you need the message after it ? or before it ? |
|
@clauz9 Ah yes. This helps. Nothing immediately stands out, but I'll compare it with the other messages, and see if there's any noticeable difference. While looking at the account settings, I noticed another thing that needs checking: Device transfer. The device transfer process copies messages and contacts, but not the registration. Instead, the receiving device is sent through a re-registration process. So, things to check: Is it possible to link a device as part of the re-registration process? Does the link complete safely and correctly? Is "transforming" a linked device to a primary via transfer a particularly bad idea? More plainly, should the "device transfer" option be removed from linked devices? If it should be, then the entire "Account" section of the settings can be removed, since pin and phone changes can't be done from a secondary |
You're right. Note that device transfer is based on the backup & restore code. Basically, device transfer streams the backup file from one device to another, using Wi-Fi direct, and then the backup is restored in the target device. Like backups, it will require re-registration, as account credentials are not saved.
If this is the case, we can replace the Account section by a help text indicating the account operations should be performed in the primary device instead. |
292807f to
ea66903
Compare
|
Added copying the QR link by tapping on the text. Could have probably done the same for tapping on the QR, but seems redundant. Fixed an issue where the device name would be reset to null after a while, due to a capabilities update re-sending a blank name. Redesigned linked devices page. Now shows the primary device with the tag "Primary", and tags the current device with "This device". Tapping on either does nothing. Unlinking a device from a linked device needs to be tested. If it doesn't work, I'll remove the option completely. Also needs testing is whether messing with Chats->Generate link previews, and Privacy->Mesaging has any effect, since in desktop, they cannot be modified. If modifying them does not work, they should be disabled. I've noticed an issue when linking where it resets the avatar. The is probably caused by the fact that RefreshOwnProfileJob starts a RetrieveProfileAvatarJob asynchronously, which causes a race condition with ProfileUploadJob, that is called by basically everything every other second for some reason. I need to figure out a work around to completely lock out profile uploads until downloading the profile is confirmed complete. Still haven't heard any test results for block-list sync, or fixed contact/group sync. |
bb65632 to
4d4f8ca
Compare
|
Fixed profile and avatar download on link. Now guaranteed to happen before a profile upload. |
|
Thank you! I released another Insider with the changes.
It was reported in Matrix. I think the fixes passes all the tests. There are more feedback in the testing room as well. We should decide whether to move all the conversation here, and tell the testers so, or try to fix the issue with Matrix. I would say try the later first but let me know what you think. |
|
Thanks I tested all the features with signal-cli in master or another android in master, and it works perfectly. I just noticed (and this is normal) that when we use parameters for the profile that are not present in the master, then the profile of the slave is always deleted. Ex : Privacy But I think it's normal because my signal-cli doesn't support these settings yet. While when the Android version supports it in master, the slave is well synchronized, except for the parameter : Find me by phone number : Nobody But ditto, this must be normal because it is not yet a parameter officially supported by Signal. In any case a huge thank you, it's a really important feature that you have enabled. I still don't understand why Signal never implemented it on Android. Anyway, thanks guys |
4d4f8ca to
007f1cb
Compare
|
Okay, this is a big one: Added support for changing phone number from primary WARNING: HIGHLY EXPERIMENTAL. The code is 90% copy pasta, and 10% absolute guesswork. When a primary changes a phone number, it regenerates the PNI and all related encryption keys. These are necessary for inter-device communication, so the number change request must be accompanied with encrypted messages containing the new keys, to be sent to each device. If the list of messages does not match the list of linked devices stored on the server, the request fails with mismatched devices error, and must be re-crafted. Any linked device must process the message and update its internal encryption keys to match the sent ones. Both signal-cli and Signal Desktop do it in a largely similar fashion, but Molly/Signal Android stores the keys in a very different way, so adjustments needed to be made accordingly. @valldrac "Fixing the issue with Matrix" would, at best, require a discussion with the matrix.org admins and/or the admins of my homeserver, and at worst, a pull request. And, for obvious reasons, I'm not planning on juggling two codebases at once. I only need the bottom line, really. Like, if you tell me "Setting pin and transferring account does not work from linked device, change the account page to a text saying 'abcdef'", and maybe put a checkbox next to it, then I'll just do it. If most of the fixes are already working, then all I have left is to disable/grey out a few primary-only features in settings, and give one more shot at pin-less group sync, for signal-cli users, since signal-cli doesn't support pin storage sync yet. @xyz-nobody Those options don't appear for me, probably because I'm using the free version. But "See my phone number" is shared through pin storage sync's account record, so signal-cli is not going to be able to interact with it, for the simple reason that it doesn't have pin storage support. On the other hand, it does mean that the setting is two-way for Android primary. By contrast, "Find me by phone number" is NOT in the pin storage. It's actually set in the account attributes call, which contains various device capabilities, including whether stories or sender key are enabled. It's not clear if a linked device can even set those, or if they're supposed to be common with the primary, or different per-device (device name is one of them, which suggests they may be per-device). This is separate from configuration sync which contains read receipts, typing indicator, unidentified delivery indicator, and link previews. Those are synchronized primary to linked only, using a simple sync message. It's not clear why Signal decided to have 3 different ways to sync configurations. Even if some are newer, it would have made sense to simply port everything to latest. P.S. I can explain why Signal never implemented this on Android (and also why this pull request is here and not upstream), but it's basically internet drama and ego wars. While some fine-tuning was done to fit the latest version, much of the code in this patch is copied or adapted from patches created back in 2016. If it was a priority, anyone could have gotten at least the basic QR-scan link process working. Even with all the adjustments made so far, I've been working on this thing around a month and a bit. That's all it takes. |
|
Definitely no lack of interest on my part! ;-) So, I had a first look at the PIN settings - which do not seem to work on linked device:
Will try to test further - but will first have to find other, non-daily used devices / signal accounts ;-) |
|
Small bug (?) : I need to force stop molly and restart it to load the list of groups I'm member of |
|
A Matrix user gave us this feedback.
|
Does changing numbers from main work? Does the linked device continue to work, or gets unlinked? -- Did not test Some other observations
Deleting messages from a linked device seems to work in a group, but not on the note to self group. The other device recieves a notification saying "[Internal-only] Recieved an invalid message!". I don't have an extra number to test with, so not sure if it works differently with multiple accounts in a group. Notes on linking a device in a secondary Android User Profile. These issues are not present when using a physical secondary device.
|
Just wanted to mention: this "does not work" with (to and from) a linked Signal Desktop (Linux / Flathub) either - so this seems to be "by design". |
|
Indeed, if you delete locally it is only deleted from the device you delete it on, not from linked devices. That has always been so. Delete for everyone does work, but of course only for a limited time and only for your own messages. |
|
Recieved an error on the linked devices when I sent something from the main molly device and seconds after delete it for all. On the main Molly device it's deleted, but not on Signal Desktop or the linked molly device with the latter giving an error notification https://debuglogs.org/android/6.23.5/a71522836c32ab4506833646851f9d990da5ae576fdb6e4535df23596ee7f114 |
|
Is this still in active development? I am using the latest apk with this on my tablet, but it is behind by a few versions now. Would love to see either an updated build or for this to be merged. |
|
yes same i use old version and its very usefull this feature
…------- Original Message -------
On Friday, August 25th, 2023 at 8:01 AM, njmdietrich ***@***.***> wrote:
Is this still in active development? I am using the latest apk with this on my tablet, but it is behind by a few versions now. Would love to see either an updated build or for this to be merged.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.[https://mail-api.proton.me/core/v4/images?Url=https%3A%2F%2Fgithub.com%2Fnotifications%2Fbeacon%2FABSH7BBGXTWEDJPDSV2B4GTXXBLUVA5CNFSM6AAAAAATCCZ3BWWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTTE5AYM4.gif&DryRun=0&UID=cmt5bxad6ng6ibm2rzllh27j7bii5mkr]
|
|
It's likely that Signal is now working on tablet support, since they'd get ranked lower on Googleplay if they don't https://community.signalusers.org/t/android-version-of-signal-now-available-on-tablets-and-chromeos/54570/7 |
|
Hey @SlugFiller, let's see if you can pick up this pull request again. Honestly, it's generated a lot of interest and I'd love to get it merged as soon as possible. In the meantime, I've gone ahead and rebased to main. I also fixed a bug related to uploading pre-signed keys at registration, due to Signal's recent updates. You can find my branch here. And for those of you who want to test and use this feature, there's a Insider release ready for download, based on v6.30.4-1: |
|
Thank you for this update, I have manually updated to v6.30.4-1+linkdevice.rev10 and so far I haven't noticed any bugs.
Tomorrow I'll continue testing
…------- Original Message -------
On Monday, August 28th, 2023 at 11:26 PM, Oscar Mira ***@***.***> wrote:
Hey @SlugFiller, let's see if you can pick up this pull request again. Honestly, it's generated a lot of interest and I'd love to get it merged as soon as possible.
In the meantime, I've gone ahead and rebased to main. I also fixed a bug related to uploading pre-signed keys at registration, due to Signal's recent updates. You can find my branch here.
And for those of you who want to test and use this feature, there's a Insider release ready for download, based on v6.30.4-1:
https://github.com/mollyim/mollyim-insider-android/releases
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.[https://mail-api.proton.me/core/v4/images?Url=https%3A%2F%2Fgithub.com%2Fnotifications%2Fbeacon%2FABSH7BBINYMJKU6CGQPRJTTXXUSJXA5CNFSM6AAAAAATCCZ3BWWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTTFD5WPY.gif&DryRun=0&UID=wcjdkleiiqiszdh3lw6jx3nrki2qbszc]
|
d29faa4 to
7c4d5d6
Compare
7c4d5d6 to
696786c
Compare
Regarding secondary android profile: Linking requires both primary and linked to be active and in the foreground in order to complete. I'm not too familiar with android multi-profile, but assuming at least one of the profile needs to be in the background, it is surprising that it works ever. Regarding pinned chats: I have difficulty testing it, since synchronizing those depends on storage sync, which doesn't work with a signal-cli main. I'll try to dig into it, but I want to know one thing first: Does it work as expected with Signal Desktop? Regarding relinking: Tested and works fine for me. Without so much as restarting the app. Certainly without clearing cache. But re-testing the link process allowed me to do some adjustments to the UI. @valldrac Looking at your rebase, it seems like you have a pretty good grasp of the code of this PR already. The differences from mine are mostly cosmetic, and it even helped me get mine done faster and/or better in some points. If you have a better setup to debug the pinned chats thing, you can probably take this over already. |
|
Hey, thanks for the detailed update and the hard work you've put into this. I'll look into the pinned chat issue as you suggested, but everything else looks good to me. I'm going to merge it. Getting all the pieces to fit together to make this feature come to life was challenging, so kudos for handling it. I'm sure the users will love this new feature. Great job! |
|
Hi all, I've been using Molly-Insider-v6.30.4-1+linkdevice.rev10 (and previous branch releases) over the last weeks. Today, I've activated Chat - Backups (in an attempt to transfer the data to newly released Molly). I don't know, whether this is related to the linkdevice functionality or a more general Molly issue. Log file at https://debuglogs.org/android/6.30.4/1ca7c546ef10d2511bd56aa49248a1a42e55719e6bfd5997d734e4ce270e11d5. |
|
I can confirm that backups fail with the latest apk from here. However, they work fine on the newest molly release on a linked device. Since I was missing old messages on the linked device anyway, I didn't mind losing the linked device's database and just started anew. |
|
Thanks, @njmdietrich, for testing (so fast)! And it's good to know that "it" (whatever the cause) has already been fixed with the official Molly release. Now that leads me to a more general question: |
|
@akki42 your suggestion intrigued me so I tested it. I was able to import a backup from my main device (which runs signal-foss) and link the device just fine, but afterwards the link seems kind of broken. I can send and receive messages from the linked device just fine, but sent messages from my main device don't sync with the linked device. I guess I will open a new issue about this Edit: turns out the same behaviour occurs when I don't import a backup. So it seems that importing backups for linked devices does not have any unwanted behaviour |
|
@njmdietrich But: Log file looks very similar to the one posted two weeks ago: Are backups still working for you on linked device? Many thanks, |
|
We're tracking the backup crash in #219. |
|
Thank You so much for that "linked device"-feature !!! |


Closes #102. Based on provisioning code from signal-cli:
https://github.com/AsamK/signal-cli/blob/master/lib/src/main/java/org/asamk/signal/manager/ProvisioningManagerImpl.java
Synchronizing contacts, groups and profile did not work in testing. But it also did not work when testing with the official Signal Desktop client, so the source of the problem seems to be elsewhere.
Some parts are a bit hacky, because device linking is stateful (requires keeping an open websocket for the entire process), while the normal registration process is stateless (Separate connection can be made for each step).