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

[Discovery] Support add user with access token #1100

Closed

Conversation

GiviMAD
Copy link

@GiviMAD GiviMAD commented Jan 13, 2023

Hi,
I have added this changes in order to support adding the client to my account using and access token with the "streaming" scope through the addUser action. It's inspired by the project https://github.com/TimotheeGerber/spotify-connect.

Do you think this can this be merged agains the 0.4.x version and included in the next 0.4.x release? Or is better that I open a pr agains the dev branch?

Hope the code is ok I don't have too much experience with rust.

Regards!

Related to:
#1086 (comment)
#1098

@kingosticks
Copy link
Contributor

Do you know what software uses the access token variant other than the spotify-connect tool?

@GiviMAD
Copy link
Author

GiviMAD commented Jan 14, 2023

No.
I want to use it for this project https://github.com/GiviMAD/openhab-addons/tree/marketplace/bundles/org.openhab.voice.habspeaker.

I think that will be also nice to allow to create the credential files manually with a Spotify token. Because right now it assumes it's a credential with password if it's not a reusable one.

But I think that this is one is more flexible as it allows an easy to implement remote way to authenticate the speaker.

Already using it and seems to work nicely.

As a side topic, the get info call returns an empty string for the field "activeUser". If we expose there the active user id, a remote system will be able to know if the speaker is already authenticated, and also to detect that the credentials are not longer valid because it can check the user id for its active credentials, this way it will now that it should remove the current speaker credentials (witch is currently not possible to do but I read some where that you can do that by sending a delete to the addUser method, so maybe was possible on other implementations or in the dev branch?) and adding the speaker to the new user account.

Does this makes sense to you?

It's actually not needed for my project because I'm doing those checks by accessing the credentials file, and I'm spawning librespot from my application so I can restart it without any trouble, but I think that is a something nice to have to allow people to handle this remotely.

Regards!

@GiviMAD
Copy link
Author

GiviMAD commented Jan 14, 2023

To give some context, in my project I keep a valid Spotify token centralized in the server (openHAB) that is obtained from a refresh token you obtain once through the oAuth Spotify authentication flow. This token is always synchronized to the connected clients (a web ui that is connected to the server through ws) so they can use the Web Playback SDK. In the electron version this same token is used to authenticate the built-in Librespot client into the user account.

@roderickvd
Copy link
Member

Hi everyone, thanks for this!

Do you think this can this be merged agains the 0.4.x version and included in the next 0.4.x release? Or is better that I open a pr agains the dev branch?

Please base it on dev. The old-api (0.4.x) is in maintenance-mode only.

As a side topic, the get info call returns an empty string for the field "activeUser". If we expose there the active user id, a remote system will be able to know if the speaker is already authenticated, and also to detect that the credentials are not longer valid because it can check the user id for its active credentials, this way it will now that it should remove the current speaker credentials (witch is currently not possible to do but I read some where that you can do that by sending a delete to the addUser method, so maybe was possible on other implementations or in the dev branch?) and adding the speaker to the new user account.

Does this makes sense to you?

Absolutely. This is also on my list, but not immediately trivial so not something I was planning to take up anytime soon. Any work on this would be highly welcomed.

@GiviMAD
Copy link
Author

GiviMAD commented Jan 14, 2023

Hi everyone, thanks for this!

Do you think this can this be merged agains the 0.4.x version and included in the next 0.4.x release? Or is better that I open a pr agains the dev branch?

Please base it on dev. The old-api (0.4.x) is in maintenance-mode only.

As a side topic, the get info call returns an empty string for the field "activeUser". If we expose there the active user id, a remote system will be able to know if the speaker is already authenticated, and also to detect that the credentials are not longer valid because it can check the user id for its active credentials, this way it will now that it should remove the current speaker credentials (witch is currently not possible to do but I read some where that you can do that by sending a delete to the addUser method, so maybe was possible on other implementations or in the dev branch?) and adding the speaker to the new user account.
Does this makes sense to you?

Absolutely. This is also on my list, but not immediately trivial so not something I was planning to take up anytime soon. Any work on this would be highly welcomed.

Sure, I'll rebase this work to dev, and I will try to add the rest of the functionality mentioned there, thank you!

@kingosticks
Copy link
Contributor

There's some more background at TimotheeGerber/spotify-connect#1

@@ -42,6 +42,14 @@ impl Credentials {
}
}

pub fn with_access_token(username: impl Into<String>, token: impl Into<String>) -> Credentials {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #1098 (comment)
I think it would be good to not enforce the username here, if possible.

Copy link
Author

@GiviMAD GiviMAD Jan 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I'll align to the work in #1098 which, I think, will add that part.

@roderickvd roderickvd added enhancement SpotifyAPI Interop b/w librespot and Spotify labels Jan 28, 2023
@GiviMAD
Copy link
Author

GiviMAD commented Jan 28, 2023

Didn't have time to look at it, I'll reopen it when I have some time, thank you for the feedback!

@GiviMAD GiviMAD closed this Jan 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement SpotifyAPI Interop b/w librespot and Spotify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants