-
Notifications
You must be signed in to change notification settings - Fork 616
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
Get access token via login5 #1344
Conversation
Great job with this. So I take it with this we can close #1220 and continue here, no? w.r.t. login5 vs. the key master, can you explain to me why from a technical viewpoint it should be one or the other and not both? If they can coexist and different downstream packages can do different things that both work... |
Yeah, i guess "from a technical viewpoint" isn't the right phrasing. Don't know what i thought there... probably sounded better or so. Anyways... i think i meant it probably in relation to the migration away from mercury to http. But yeah "technically" there isn't anything speaking against it letting it stay. Was probably to much in my thoughts so i didn't question my own phrasing anymore. But probably in favor of migration away from mercury, removing the keymaster token retrieval or at least marking it as deprecated would probably be in the favor of the overall process of the migration, i suppose? |
Btw. do we still want to add this to 0.5? If not, i would probably put the PR in a draft state until 0.5 is released. |
As-is, it's just extra, right? With the librespot binary still getting tokens through the keymaster? If so, I'd be fine with putting it in v0.5 if people agree the code is good. If it replaces the current token getter (but I don't think so?) then yeah maybe merge right after a v0.5 release and let it get a little more exposure. |
As far as i am aware, the remaining usage of the keymaster token retrieval is only in the But we could introduce a new feature and either make login5 totally optional or just switch the token retrieval parts. With that we could merge it in 0.5 without breaking anything and proving people who need it the option to opt-in. |
I think (I've never tried), one way to get a hashcash challenge is to use credentials.password (on android) instead of stored credentials. However, this code as is, isn't really a generic login5 implementation that downstream library users could use to login with. The way we're using it is just as a tokenmaster replacement. This is fine, but this limitation is worth keeping in mind. |
I'm pretty sure I triggered the hashcash using zeroconf while acting as an Android or iOS client. Re: @photovoltex maybe I'm senile 😆 but this PR does nothing to move from key master to login5 right? I'm still a bit confused whether this needs more shakedown or not. Re: #1331 in |
I tried it again with |
You can also fool it into thinking you're device XYZ by forcing it in several places in the code. |
Okay. So i tried some things and got the client_token to send me a hash cash challenge, but login5 just responses with "invalid credentials" if i send the android or ios client_id with my stored credentials. As it seems in the java implementation they also only did send the keymaster_client_id. So maybe we should do the same here? I also noticed that we don't await the client token request, and by that request the client token twice initially: Because this is cause by this PR i could try to find a fix for it, or we move it to a new issue after we merged this? |
Sorry, can't remember anymore what triggered it then. Is Spotify still using it at all?
Tough to say. One the one hand, while
Would be great if you could get it in here. |
Well for the client token it seems so. For login5... good question, will see what i can figure out.
I usually get a bit lost when trying to analyze the desktop client, so i tend to using the web-player to get information.
Sounds good. Will see what i can do and report back, and or just fix it. |
So I'm back with some news on the topic, after investigating some network traffic. TL;DR: Lin and Win seem to use the oauth2 to login, but on android you don't leave the app, so no oauth2 is used and instead login5 with user password authentication. The first response includes an hashcash challenge. The next request has the initial request data (so user and pwd credentials) in addition to the solved challenge. And the next response includes: the access token and the stored credentials. I tried sending the user/pwd via login5 on linux and it seems by that we are presented with a hashcash challenge. But the current hash cash algo doesn't seem to work here? Will investigate and hopefully fix that. For android it might be that only the stored credentials retrieved by login5, will work to refresh the token, but i need to investigate that further to give actual infos on that^^. But if we merge this current state the android side of the library might break, because of the above mentioned assumption. |
But it seems to be the same on all platforms, right? What error did you get? |
Well I only tried the login via login5 on linux once so far. I didn't dig much deeper on it and why the hashcash failed to be solved. But was planing to do that after fixing the duplicate token requests. |
There are usually two requirements, the first is to solve the hashcash quickly (less than 5 seconds), the second is to send it in upper-case or it might be rejected, I think that's it... maybe it will be useful |
Good thing to point out! But sadly not the solution or the right direction. Initial i forgot to add the login context at the re-request, thats why the initial login didn't work and it looked like the hashcash algo was incorrect. I tried a bit and compared results and with that i can confirm that our hashcash algo is correct and gives us the same result as the android client version. For the login5 response we work directly on the bytes, so no upper-case or conversion to string is required. Anyhow, i still get an |
Okay got it to work. Login via login5 seems to be only available for mobile (or at least android). It works after adjusting the user-agent and the client-id. So that the client-id for android has an android user-agent. And i can also now confirm that the login5 stored credentials are required to request a login5 token again with the android client-id and user-agent |
Good going. Something you could document in the source for future reference? By the way, I found that it works a lot more often in release profile than in debug profile, because the computation is much faster that way. |
Just tried the whole thing for ios, and that also seems to work :D
If i don't modify the client token request (which doesn't need to be requests by ios and android) and by that skip the client_token hashcash challenge, it's quite fast for me so debug or release doesn't matter much.
Will see what makes sense to document, I'm also kinda tempted for an mobile mock mode, but will probably offload that into another PR (if i even do it at all) because that would be a bit to much for this PR. |
So... this should be pretty good state of things. This PR now implements the access-token retrieval and login for mobile. I didn't implement any option in librespot's main function. For that i would like to know if there is even a necessity to do so? So if there are any users who use the librespot binary on mobile? Otherwise i think the exposed method should be enough. |
The binary on mobile? The library yes but the binary I don't know about. I'd say that people can contribute themselves if they have a need for that. I plan to merge this right after releasing v0.5.0 and have it get some amount of shakedown. |
Sounds like a good plan to me. Thanks again for taking your time on the topic :). Fun fact: after we merged the dealer and this PR, the mobile version will be in a state where mercury isn't required anymore. For desktop on the other hand, i think we are still missing the initial stored credentials retrieval (which we now have for mobile with login5). But i guess that's a topic to investigate on its own^^ |
Even mobile will still need Mercury to get the decryption keys, right? |
Can you resolve the conflict in |
Ah dang, forgot that we get the keys via mercury. So yeah mercury is still required then either way. But good to know that i would need to look at the
Sure thing
Wanted to write: Don't we need a new version for that first? But nice nice. Thanks for handling the whole release part^^. Will try to add a fitting entry. |
99cd7ec
to
c157420
Compare
There we go. I cleaned the code a bit up. Did improve the error handling, removed unnecessary clones wherever possible and adjusted the fix for the duplicated request. Did just feel wrong to add a semaphore to fix that problem. Anyhow. In regards to the changelog. For the I might look into it tomorrow, but even when it does break the requests, would it be seen as a breaking change, or just broken credentials cause of spotify? |
I wouldn't say that it's breaking, because it doesn't change an existing API. |
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.
Some points after having reviewed your latest cleanup.
Merged 🚀 |
As stated in #1220 (comment) the PR to implement the login5 token retrieval into dev. I couldn't test if solving the
challenge
works, because it didn't send any challenge to solve (i tested it connecting from Android and Linux, so maybe win or osx could do the trick). But if solving thehashcash
is universal and solving thehashcash
for theclient_token
works, this should as well.The general flow is copied fromgo-librespot
.In #1309 (comment) we already had the problem mentioned with having two solutions to retrieve a token. From the technically viewpoint i would like to remove the
TokenProvider
completely in favor of login5. But sadly there are some arguments for letting it stay. Compared to login5 the keymaster can provide tokens in relation to a custom client_id. Login5 requests just give aBAD_REQUEST
error if we try do request a token with a custom client_id. I know that some applications likencspot
use this token acquisition to avoid doing the general oauth flow and or storing the client_secret in the application.So i would like some feedback how we should proceed with that problem. It's probably also quite dependent if we still merge this into 0.5 or not?
Fixes #1179
Fixes #1245