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

Get access token via login5 #1344

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

photovoltex
Copy link

Based on @kingosticks work on implementing login5, see kingosticks/login5-again

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 the hashcash is universal and solving the hashcash for the client_token works, this should as well. The general flow is copied from go-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 a BAD_REQUEST error if we try do request a token with a custom client_id. I know that some applications like ncspot 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

@roderickvd
Copy link
Member

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

@photovoltex
Copy link
Author

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?

@photovoltex
Copy link
Author

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.

@roderickvd
Copy link
Member

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.

@photovoltex
Copy link
Author

photovoltex commented Sep 21, 2024

As far as i am aware, the remaining usage of the keymaster token retrieval is only in the get_token example.

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.

@kingosticks
Copy link
Contributor

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.

@roderickvd
Copy link
Member

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 dev I changed it to take the client_id we got from spirc because I thought it was the right thing to do. But who's to say my thinking was totally off.

@photovoltex
Copy link
Author

I'm pretty sure I triggered the hashcash using zeroconf while acting as an Android or iOS client.

I tried it again with cargo run -- -z 12345 --verbose (might be that i didn't use it correct) and connected from my android device, but no hashcash there. Neither for the client token or the login5 token. Anyhow i may have access to an ios and osx device the following days (probably Thursday) and will test it then again. Will hopefully come back with good news that the code just works^^

@roderickvd
Copy link
Member

You can also fool it into thinking you're device XYZ by forcing it in several places in the code.
From memory, things that depend on it are the version string, Mercury auth, HTTP headers, and client ID.

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.

Error 403
3 participants