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

Registration fixes for 2023 #238

Merged
merged 14 commits into from Aug 7, 2023
Merged

Registration fixes for 2023 #238

merged 14 commits into from Aug 7, 2023

Conversation

rubdos
Copy link
Member

@rubdos rubdos commented Jul 25, 2023

The registration procedure has changed a few months ago, and the old API has been terminated. This is an attempt to expose the new registration API surface for Whisperfish. All the other clients I know are secondary-only, so this shouldn't affect anyone but Whisperfish.

@rubdos rubdos marked this pull request as draft July 25, 2023 12:03
@rubdos rubdos force-pushed the registration-2023 branch 2 times, most recently from 66f1604 to 30a1ec7 Compare July 26, 2023 11:57
@rubdos
Copy link
Member Author

rubdos commented Jul 26, 2023

@MarcusGrass
Copy link
Contributor

Don't know if this is related to this specific PR, but it seems that https://github.com/whisperfish/libsignal-service-rs/blob/main/libsignal-service/src/provisioning/manager.rs#L173 is now a raw string Ex Got provisioning code: 'ac9e637e-16d1-50bd-b0aa-daf33b346cb0.1691236722275:7U2fte60zkioI3KLaIEZQT2skhYgFXLjIL2hgQqqLAQ=' Interestingly that worked yesterday

@rubdos
Copy link
Member Author

rubdos commented Aug 5, 2023

That would indeed be related, thanks for reporting. I have not been able to test this yet, because I don't have access to my second SIM.

@MarcusGrass
Copy link
Contributor

That would indeed be related, thanks for reporting. I have not been able to test this yet, because I don't have access to my second SIM.

Yeah it's not easy with only one number, threw up my local fix as a separate PR here #240, it's nice in that it's just skipping some validation and shouldn't mess with any compatibility if someone were to unexpectedly have some old signal-compatible server they were testing against.

@rubdos
Copy link
Member Author

rubdos commented Aug 5, 2023

Thanks! I've cherry-picked your commit into this PR. Were you able to confirm a device with the other patches in here?

@MarcusGrass
Copy link
Contributor

Thanks! I've cherry-picked your commit into this PR. Were you able to confirm a device with the other patches in here?

Nice, I can't test the registration flow at the moment because I also need another SIM, also, I'm using https://github.com/whisperfish/presage/ which breaks from the api-change here, would need to figure out how to rewire it first, do you know where the current sources for the java sdk is? All I can find are archived repos.

But the link-device flow works after that change on this branch!

I am getting this in the logs though:

[2023-08-05T13:48:47Z ERROR libsignal_service::websocket] SignalWebSocket: Websocket error: end of web request stream; socket closing
[2023-08-05T13:48:51Z ERROR libsignal_service::websocket] SignalWebSocket: Websocket error: SignalWebSocket: end of application request stream; socket closing

It seems that the socket is closed from Signal's side https://github.com/whisperfish/libsignal-service-rs/blob/main/libsignal-service/src/websocket.rs#L231, then the stream starts returning None, although I don't think that's necessarily unexpected in this case since the connection is created and dropped for this request.

@rubdos
Copy link
Member Author

rubdos commented Aug 5, 2023

do you know where the current sources for the java sdk is? All I can find are archived repos.

Everything is in Signal-Android these days. @gferon usually patches presage within a few days after I push things in here.

It seems that the socket is closed from Signal's side https://github.com/whisperfish/libsignal-service-rs/blob/main/libsignal-service/src/websocket.rs#L231, then the stream starts returning None, although I don't think that's necessarily unexpected in this case since the connection is created and dropped for this request.

I think there's an issue about this. Because it doesn't necessarily harm the procedure, we didn't really bother at that point...

@MarcusGrass
Copy link
Contributor

do you know where the current sources for the java sdk is? All I can find are archived repos.

Everything is in Signal-Android these days. @gferon usually patches presage within a few days after I push things in here.

Ah nice, thanks!

It seems that the socket is closed from Signal's side https://github.com/whisperfish/libsignal-service-rs/blob/main/libsignal-service/src/websocket.rs#L231, then the stream starts returning None, although I don't think that's necessarily unexpected in this case since the connection is created and dropped for this request.

I think there's an issue about this. Because it doesn't necessarily harm the procedure, we didn't really bother at that point...

Makes sense, it doesn't really seem problematic

@gferon
Copy link
Collaborator

gferon commented Aug 5, 2023

That would indeed be related, thanks for reporting. I have not been able to test this yet, because I don't have access to my second SIM

FWIW, did you think of trying with your main SIM on staging servers? You could re-register infinitely (don't quote me on that) on the test servers.

@rubdos
Copy link
Member Author

rubdos commented Aug 5, 2023

Mea culpa, the confirm code was not part of the registration procedure at all. Any way useful to get it in here, it's part of registration anyway.

@rubdos
Copy link
Member Author

rubdos commented Aug 7, 2023

@direc85 confirms this works for Whisperfish.

@rubdos rubdos requested a review from gferon August 7, 2023 09:40
@rubdos rubdos marked this pull request as ready for review August 7, 2023 09:40
@rubdos
Copy link
Member Author

rubdos commented Aug 7, 2023

The registration session thingy would be really cool in a nice RegistrationSession state machine structure, but I'm a bit too lazy to actually implement that now... Updating the examples.

@rubdos
Copy link
Member Author

rubdos commented Aug 7, 2023

@gferon this should be it. If you can throw some secondary eyes on it, feel free to hit merge!

@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Merging #238 (43b2d4f) into main (6d2649d) will decrease coverage by 0.34%.
Report is 2 commits behind head on main.
The diff coverage is 0.00%.

@@           Coverage Diff            @@
##            main    #238      +/-   ##
========================================
- Coverage   4.16%   3.82%   -0.34%     
========================================
  Files         36      36              
  Lines       2523    2746     +223     
========================================
  Hits         105     105              
- Misses      2418    2641     +223     
Files Changed Coverage Δ
libsignal-service-actix/src/push_service.rs 9.83% <0.00%> (-2.21%) ⬇️
libsignal-service-hyper/src/push_service.rs 16.31% <0.00%> (-2.55%) ⬇️
libsignal-service/src/provisioning/manager.rs 0.00% <ø> (ø)
libsignal-service/src/provisioning/mod.rs 0.00% <ø> (ø)
libsignal-service/src/push_service.rs 0.00% <0.00%> (ø)

@@ -227,6 +232,85 @@ pub struct WhoAmIResponse {
pub number: PhoneNumber,
}

#[derive(Debug, Clone, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct RegistrationSessionMetadataResponse {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Java :')

Copy link
Collaborator

@gferon gferon left a comment

Choose a reason for hiding this comment

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

LGTM! I'll adapt presage immediately and check that it works as expected (although I read on the chat that somebody tried already.)

@gferon gferon merged commit 8789920 into main Aug 7, 2023
10 of 11 checks passed
@gferon gferon deleted the registration-2023 branch August 7, 2023 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants