Kingosticks change: Obtain spclient access token using login5 instead of keymaster (Fixes librespot-org#1179) #1220

Gerrelt · 2023-11-13T20:54:16Z

@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.

… of keymaster (Fixes librespot-org#1179)

roderickvd · 2023-11-14T20:14:09Z

Thanks for making the effort.

Let's first get to ticking the first two checkboxes:

Getting the build to succeed
Confirming the compatibility questions stated in Error 403 #1179

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.

… of keymaster (Fixes librespot-org#1179)

roderickvd · 2023-12-03T19:34:36Z

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

Gerrelt · 2023-12-11T11:07:23Z

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.

kingosticks · 2023-12-11T11:10:08Z

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.

Gerrelt · 2023-12-11T11:12:54Z

OK, I will test that scenario.

Gerrelt · 2023-12-11T18:51:47Z

I've did a test with an iPhone 12, with librespot still on a Raspberry Pi.

connected iPhone, started a song, it started playing on the Pi
skipped to middle of song
skipped to next song
skipped to nearly the end of the song, and let it play into the next song.
This all went without problems.

Then I switched the Raspberry Pi off, and we saw the spotify connect device disappear on the iphone.
Started the Pi again, and connected the iPhone again.
We then could succesfully repeat the steps 1 - 4 again as described above.
So, seems to work ok?

herrernst · 2023-12-11T19:47:13Z

Then I switched the Raspberry Pi off, and we saw the spotify connect device disappear on the iphone. Started the Pi again, and connected the iPhone again. We then could succesfully repeat the steps 1 - 4 again as described above. So, seems to work ok?

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.)

Gerrelt · 2023-12-12T13:31:11Z

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.
iPhone (or Android device) is logged in into Spotify, and then it connects to the librespot spotify connect device on the Pi.

roderickvd · 2023-12-15T11:30:26Z

Great work. I’m not entirely on top and cannot test myself. Will you give me the go ahead if I can merge?

Gerrelt · 2023-12-15T12:08:38Z

Yes, you can merge it.

kingosticks · 2023-12-15T12:22:14Z

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?

kingosticks · 2023-12-15T12:27:28Z

core/src/spclient.rs

+                break message.ok().to_owned();
+            }
+
+            if count < MAX_TRIES {


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; }

At the very least, a retry in the face of BAD_REQUEST or a TOO_MANY_ATTEMPTS error response is a bit rude.

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?

kingosticks · 2023-12-15T12:28:19Z

core/src/spclient.rs

+                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"


As is, there is still no hash cash handling so this error message makes no sense.

kingosticks · 2023-12-15T12:36:09Z

core/src/spclient.rs

-                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)?);


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...

kingosticks · 2023-12-15T12:40:49Z

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.

kingosticks · 2023-12-15T13:08:26Z

core/src/spclient.rs

-                .token_provider()
-                .get_token("playlist-read")
-                .await?;
+            let auth_token = self.auth_token().await?;


~~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"...

roderickvd · 2024-01-03T19:56:19Z

bump if you could take a look at resolving @kingosticks's review comments? ❤️

roderickvd · 2024-03-31T09:34:53Z

Am I right that this one can be closed in favour of #1246?

hamishfagg · 2024-05-16T09:56:43Z

FWIW I get the following errors when trying to use this branch now (when trying to play a track):

2024-05-16 09-55-11.596 [Error] (librespot_playback::player) Unable to load audio item: Error { kind: InvalidArgument, error: StatusCode(400) }
2024-05-16 09-55-11.597 [Error] (librespot_playback::player) Skipping to next track, unable to load track <SpotifyId("spotify:track:7mDKRYiqejoHzP7dQGxLys")>: ()
2024-05-16 09-55-11.796 [Error] (librespot_playback::player) Unable to load audio item: Error { kind: InvalidArgument, error: StatusCode(400) }

roderickvd · 2024-08-21T19:49:13Z

Can we deprecate in favour of #1309?

roderickvd · 2024-09-13T05:43:35Z

Could you comment on the status of this? Do we want it in yes/no? @kingosticks

photovoltex · 2024-09-13T06:24:20Z

Hey just to buzz in. I'm currently trying to replace mercury with the dealer/websocket for the spirc. I got stuck on putting the connect state for some days.

The solution in the end was it to merge @kingosticks login5 impl, and use that token for both the dealer and internal api request. So it would be much appreciated if we can get the login5 auth merged at some point :D

roderickvd · 2024-09-13T06:40:41Z

You working on the dealer? Hero! Open a draft PR if you want some community input.

So you're saying this PR is good to merge?

photovoltex · 2024-09-13T07:20:29Z

the general implementation of login5 is required to continue integrating the dealer, yeah

in regards to the dealer integration, i still have some stuff to do before i can make a pr, for now i need to have a consistent way to setup the dealer, currently it's still inconsistent putting the connect state and retrieving the cluster update. go-librespot seems to achieve such thing, but for my current impl it still seems to have an inconsistent behavior.

like for example a cold start works, restarting librespot right after, will not put the connect state successfully (the request is successful, but the dealer will not receive cluster updates)... until i have figured that out i will probably just experiment locally

roderickvd · 2024-09-14T16:45:28Z

Thanks for the update.

More specifically, my question was about the readiness of this PR. Can it be merged yes/no?

photovoltex · 2024-09-14T19:00:02Z

Oh sry misunderstood that.

Well the points @kingosticks pointed out are still valid. It would probably not be the end of the world to merge these, but fixing them beforehand is probably preferable. Besides with conflicts we can't even merge it, even if we would want to.

Because this PR seems pretty abandon maybe closing it and reopening might be the better way to go. I could take the changes again and clean them up like @kingosticks requested? Unless he wants to do that himself.

photovoltex · 2024-09-15T19:58:32Z

sooo... as it seems login5 isn't required for the dealer. The old impl (mercury) and new impl (dealer) are conflicting with each other and sometime the new impl wins but most of the time the old wins.. retrieving a token via login5 probably delays the new method enough so that it has a higher change of winning... pretty silly reasoning, but looking back on it, it make a lot of sense

just wanted to clear up my misunderstanding

even tho i don't necessary need these changes anymore, my offer in regards to bringing them into dev still applies

kingosticks · 2024-09-16T15:37:04Z

I sorted the conflicts in https://github.com/kingosticks/librespot/tree/login5-again

@photovoltex if you're intersted in taking it from half-baked proof-of-concept into something more mergable (e.g. sort the error handling) I would love that. Otherwise, given we don't seem to actually NEED it for a 0.5 release, I'll get to it at some point...

roderickvd · 2024-09-16T17:40:46Z

OK, so I'll wait until someone gives me the sign and won't target it for 0.5.

photovoltex · 2024-09-16T18:01:04Z

Already did some adjustments here https://github.com/photovoltex/librespot/tree/login5-again, if anyone wants to take a look. I moved the impl into an own file and manger. And additionally handled the errors and solved the response challenges for the retries.

I move the login5 code in an own file because it is only used by the spclient and doesn't use any endpoint of it.

I would like to do the same for the client-token retrieval and other methods that are unrelated to the spclient, but thats probably a point for another day.

I may open a PR in the next days. Unless someone takes the branch and opens a PR before me

roderickvd · 2024-09-19T20:04:41Z

Continuing in #1344.

Kingosticks change: Obtain spclient access token using login5 instead…

96e02fd

… of keymaster (Fixes librespot-org#1179)

Gerrelt mentioned this pull request Nov 13, 2023

Error 403 #1179

Closed

roderickvd added SpotifyAPI Interop b/w librespot and Spotify new api labels Nov 15, 2023

mbeijen mentioned this pull request Nov 18, 2023

Couldn't establish a new session. MercuryClient$MercuryException: status: 403 librespot-org/librespot-java#697

Open

roderickvd linked an issue Nov 19, 2023 that may be closed by this pull request

Error 403 #1179

Closed

Gerrelt added 2 commits December 1, 2023 20:37

Kingosticks change: Obtain spclient access token using login5 instead…

ba08273

… of keymaster (Fixes librespot-org#1179)

Merge branch 'librespot-org:dev' into auth-token-from-login5

f065908

Gerrelt added 4 commits December 4, 2023 20:28

Merge branch 'librespot-org:dev' into auth-token-from-login5

a54cc8f

Kingosticks change: Obtain spclient access token using login5 instead…

f815318

… of keymaster (Fixes librespot-org#1179)

Merge branch 'librespot-org:dev' into auth-token-from-login5

40c2a4d

Merge branch 'auth-token-from-login5' of https://github.com/Gerrelt/l…

87dddf2

…ibrespot into auth-token-from-login5 Pulling commit: fix startup log

kingosticks reviewed Dec 15, 2023

View reviewed changes

lmore377 mentioned this pull request Jan 19, 2024

Authentication on all desktop platforms is broken #1245

Closed

lmore377 mentioned this pull request Jan 19, 2024

Obtain spclient access token using login5 instead of keymaster (Fixes #1179 and #1245) #1246

Closed

DaXcess added a commit to SpoticordMusic/librespot that referenced this pull request Jul 1, 2024

Merge librespot-org#1220 from upstream PRs

0c7b50e

DaXcess mentioned this pull request Jul 1, 2024

regression: lyrics API no longer working #1281

Closed

photovoltex mentioned this pull request Sep 19, 2024

Get access token via login5 #1344

Merged

roderickvd closed this Sep 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kingosticks change: Obtain spclient access token using login5 instead of keymaster (Fixes librespot-org#1179) #1220

Kingosticks change: Obtain spclient access token using login5 instead of keymaster (Fixes librespot-org#1179) #1220

Gerrelt commented Nov 13, 2023

roderickvd commented Nov 14, 2023

roderickvd commented Dec 3, 2023

Gerrelt commented Dec 11, 2023 •

edited

Loading

kingosticks commented Dec 11, 2023 •

edited

Loading

Gerrelt commented Dec 11, 2023

Gerrelt commented Dec 11, 2023 •

edited

Loading

herrernst commented Dec 11, 2023

Gerrelt commented Dec 12, 2023 •

edited

Loading

roderickvd commented Dec 15, 2023

Gerrelt commented Dec 15, 2023

kingosticks commented Dec 15, 2023

kingosticks Dec 15, 2023

kingosticks Dec 15, 2023 •

edited

Loading

kingosticks Dec 15, 2023 •

edited

Loading

kingosticks Dec 15, 2023

kingosticks Dec 15, 2023 •

edited

Loading

kingosticks commented Dec 15, 2023

kingosticks Dec 15, 2023 •

edited

Loading

roderickvd commented Jan 3, 2024

roderickvd commented Mar 31, 2024

hamishfagg commented May 16, 2024 •

edited

Loading

roderickvd commented Aug 21, 2024

roderickvd commented Sep 13, 2024

photovoltex commented Sep 13, 2024

roderickvd commented Sep 13, 2024

photovoltex commented Sep 13, 2024

roderickvd commented Sep 14, 2024

photovoltex commented Sep 14, 2024

photovoltex commented Sep 15, 2024

kingosticks commented Sep 16, 2024

roderickvd commented Sep 16, 2024

photovoltex commented Sep 16, 2024

roderickvd commented Sep 19, 2024

Kingosticks change: Obtain spclient access token using login5 instead of keymaster (Fixes librespot-org#1179) #1220

Kingosticks change: Obtain spclient access token using login5 instead of keymaster (Fixes librespot-org#1179) #1220

Conversation

Gerrelt commented Nov 13, 2023

roderickvd commented Nov 14, 2023

roderickvd commented Dec 3, 2023

Gerrelt commented Dec 11, 2023 • edited Loading

kingosticks commented Dec 11, 2023 • edited Loading

Gerrelt commented Dec 11, 2023

Gerrelt commented Dec 11, 2023 • edited Loading

herrernst commented Dec 11, 2023

Gerrelt commented Dec 12, 2023 • edited Loading

roderickvd commented Dec 15, 2023

Gerrelt commented Dec 15, 2023

kingosticks commented Dec 15, 2023

kingosticks Dec 15, 2023

Choose a reason for hiding this comment

kingosticks Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

kingosticks Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

kingosticks Dec 15, 2023

Choose a reason for hiding this comment

kingosticks Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

kingosticks commented Dec 15, 2023

kingosticks Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

roderickvd commented Jan 3, 2024

roderickvd commented Mar 31, 2024

hamishfagg commented May 16, 2024 • edited Loading

roderickvd commented Aug 21, 2024

roderickvd commented Sep 13, 2024

photovoltex commented Sep 13, 2024

roderickvd commented Sep 13, 2024

photovoltex commented Sep 13, 2024

roderickvd commented Sep 14, 2024

photovoltex commented Sep 14, 2024

photovoltex commented Sep 15, 2024

kingosticks commented Sep 16, 2024

roderickvd commented Sep 16, 2024

photovoltex commented Sep 16, 2024

roderickvd commented Sep 19, 2024

Gerrelt commented Dec 11, 2023 •

edited

Loading

kingosticks commented Dec 11, 2023 •

edited

Loading

Gerrelt commented Dec 11, 2023 •

edited

Loading

Gerrelt commented Dec 12, 2023 •

edited

Loading

kingosticks Dec 15, 2023 •

edited

Loading

kingosticks Dec 15, 2023 •

edited

Loading

kingosticks Dec 15, 2023 •

edited

Loading

kingosticks Dec 15, 2023 •

edited

Loading

hamishfagg commented May 16, 2024 •

edited

Loading