-
Notifications
You must be signed in to change notification settings - Fork 593
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
base: dev
Are you sure you want to change the base?
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. |
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