-
Notifications
You must be signed in to change notification settings - Fork 528
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
Kingosticks change: Obtain spclient access token using login5 instead of keymaster (Fixes librespot-org#1179) #1220
base: dev
Are you sure you want to change the base?
Conversation
… of keymaster (Fixes librespot-org#1179)
Thanks for making the effort. Let's first get to ticking the first two checkboxes:
Overall I really like that this moves away from the Mercury keymaster and towards a HTTP endpoint. That's where this should go. For bonus points you could look into also fixing the Mercury keymaster request. The solution probably lies in inspecting the traffic from the official client, and get the combination of HTTP headers, protobuf request, and keymaster ID precisely right per platform. The official clients do it differently between Windows, Linux, macOS, iOS and Android. When I was an active developer I reverse engineered everything but only tested Linux and macOS. |
Good to see work on it again! Let's resolve those clippy warnings 👍 |
… of keymaster (Fixes librespot-org#1179)
…ibrespot into auth-token-from-login5 Pulling commit: fix startup log
Hello Roderick, I've solved the fmt and clippy warnings, could you review te change again? This librespot version, with the latest changes, was tested on a raspberry pi, and it seems to work fine. |
The remaining checkbox blocking this is to confirm/deny the the iOS problems that were reported at #1179 (comment) I don't have any iOS devices, I can't help. |
OK, I will test that scenario. |
I've did a test with an iPhone 12, with librespot still on a Raspberry Pi.
Then I switched the Raspberry Pi off, and we saw the spotify connect device disappear on the iphone. |
You don't have username and password hardcoded as librespot params, or do you? If not (i.e. if iPhone initiates login and that works), it's good. (Unfortunately I can't test because I don't have an active subscription.) |
No, I don't have the username and password hardcoded as params. |
Great work. I’m not entirely on top and cannot test myself. Will you give me the go ahead if I can merge? |
Yes, you can merge it. |
I never implemented the hash cash stuff for this. In what situation is that used? Have we missed a test case or is it just not required here? |
break message.ok().to_owned(); | ||
} | ||
|
||
if count < MAX_TRIES { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this would be improved to check for the error cases where a retry is not sensible:
enum LoginError {
UNKNOWN_ERROR = 0;
INVALID_CREDENTIALS = 1;
BAD_REQUEST = 2;
UNSUPPORTED_LOGIN_PROTOCOL = 3;
TIMEOUT = 4;
UNKNOWN_IDENTIFIER = 5;
TOO_MANY_ATTEMPTS = 6;
INVALID_PHONENUMBER = 7;
TRY_AGAIN_LATER = 8;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the very least, a retry in the face of BAD_REQUEST
or a TOO_MANY_ATTEMPTS
error response is a bit rude.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A simple exponential backup would be nice too, ideally implemented in a reusable way. But could leave that to another day, I'm just trying to keep a record of what was left unfinished.
EDIT: Actually, there is a solid handling of the HTTP side of this in our HttpClient
. Given this is protobuf over HTTP, it depends what Spotify do. i.e. do they reflect protobuf errors in the HTTP response? I don't think I ever looked into that. @roderickvd do you know?
response = self.auth_token_request(&login_request).await?; | ||
} else { | ||
return Err(Error::failed_precondition(format!( | ||
"Unable to solve any of {MAX_TRIES} hash cash challenges" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As is, there is still no hash cash handling so this error message makes no sense.
warn!("Unable to get client token. Trying to continue without..."); | ||
} | ||
let client_token = self.client_token().await?; | ||
headers_mut.insert(CLIENT_TOKEN, HeaderValue::from_str(&client_token)?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes how we handle the failing case. The old code used to continue anyway, maybe that's still what we want. I don't remember why I thought I had to change it...
I am sorry for my late review. I should have highlighted more clearly (and earlier) the bits I didn't finish. It obviously all works as is but it could be improved on. |
.token_provider() | ||
.get_token("playlist-read") | ||
.await?; | ||
let auth_token = self.auth_token().await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also probably check the result of this and break the loop if it failed.
EDIT: Cancel that, I forgot what ?
does. But maybe that's actually the wrong thing to do given "currently these endpoints seem to work fine without it"...
bump if you could take a look at resolving @kingosticks's review comments? ❤️ |
Am I right that this one can be closed in favour of #1246? |
FWIW I get the following errors when trying to use this branch now (when trying to play a track):
|
See issue ccomment:
#1179 (comment)
@kingosticks asked if somebody else wanted to create the pull-request.
I've added the changes to the latest dev branch of librespot, compiled and tested it on raspberry pi.
It seems to still solve the problem as addressed in issuee 1179, it's working fine.
But I just copied the changes from kingostick, so all the questions, and the honor ( :-) ) are for kingosticks!
This is my attempt at creating a pull request. I've only done it once before, so I hope it's done correctly.