Use Oauth2 login with spotify #731

stevenleadbeater · 2024-10-27T19:43:50Z

fixes #729, fixes #728 and fixes #726

works for me locally, I preserved the original login method, added a new one for login with spotify and fixed an issue that stopped tracks playing. I'm not massively familiar with this code (I pulled it yesterday because I wanted it to work on my pinephone) I'm happy to do a bit more to get this mergeable if needed but please bear in mind I may need a bit of guidance on what files to look at to solve issues

…ifyId::from_base62` is setting this to unkown which causes playback to fail with ` Unable to load audio item: Error { kind: Unavailable, error: NonPlayable } / unable to load track <SpotifyId("spotify:unknown:1iI5J72TQxYdQkKnkRwWCn")>`

…e URL in the default browser

stevenleadbeater · 2024-10-28T09:50:26Z

That flatpak builder failure looks like clippy complaining about pre-existing code. Is this a new thing?

stevenleadbeater · 2024-10-28T16:02:46Z

tested it on my pinephone too and I have tunes :)

schooldanlp6

but it should work with librespot 0.6.0 too which is also what I run on my OS currently

but thanks for the fix it works now :))))

stevenleadbeater · 2024-11-09T08:55:45Z

but it should work with librespot 0.6.0 too which is also what I run on my OS currently

but thanks for the fix it works now :))))

You must have commented 0.6.0 within days of it's release :') I'll give it a go and see how it works. I was actually looking at the login5 stuff on librespot too as it may fix the other authentication issue....

alumni · 2024-11-10T18:15:24Z

I also tested it locally and it seems it's working quite well. I would remove the old login button.

@xou816 Could you review?

xou816 · 2024-11-10T18:54:50Z

Looks like a lot of great work, I'll try to review that tomorrow, thanks!

xou816 · 2024-11-12T00:20:26Z

Looks good overall! I would have a few things to nitpick but nothing major/no reason to delay this. Just hesitant about the client id change, is it needed?

xou816

(nothing blocking, just want to discuss the client id real quick)

xou816 · 2024-11-12T00:16:23Z

src/player/player.rs

@@ -278,7 +311,8 @@ impl SpotifyPlayer {
    }
 }

-const CLIENT_ID: &str = "782ae96ea60f4cdf986a766049607005";
+const REDIRECT_URI: &str = "http://127.0.0.1:8898/login";
+const SPOTIFY_CLIENT_ID: &str = "65b708073fc0480ea92a077233ca87bd";


does the client id need to change? i guess we should eventually make this configurable

updated my client to allow the expected redirect uris, if you want to revert that!

xou816 · 2024-11-12T00:30:25Z

src/app/components/login/login.blp

+
+          Button login_with_spotify_button {
+            /* Translators: Log in button label */
+            label: _("Log in with Spotify");


the wording could probably be improved to make it clear its an external login, but I dont have any great suggestion just yet; probably need to review the whole ui of this dialog!

xou816 · 2024-11-12T00:31:02Z

src/api/oauth2.rs

+        .url();
+
+    println!("Browse to: {}", auth_url);
+    if let Err(err) = open::that(auth_url.to_string()) {


I'm not super clear on the benefits, but I'd be interested in doing that with portals/ASPHD at some later time!

xou816 · 2024-11-12T00:32:04Z

src/api/oauth2.rs

+    }?;
+    trace!("Exchange {code:?} for access token");
+
+    // Do this sync in another thread because I am too stupid to make the async version work.


that's absolutely fine that can be adressed later 😄

xou816 · 2024-11-12T00:32:26Z

src/api/oauth2.rs

+    };
+    let refresh_token = match token.refresh_token() {
+        Some(t) => t.secret().to_string(),
+        _ => "".to_string(), // Spotify always provides a refresh token.


should this trigger an error then?

xou816 · 2024-11-12T00:33:35Z

src/api/oauth2.rs

+
+    let token_scopes: Vec<String> = match token.scopes() {
+        Some(s) => s.iter().map(|s| s.to_string()).collect(),
+        _ => scopes.into_iter().map(|s| s.to_string()).collect(),


i guess its technically nullable but not really in practice, maybe it should be an error as well?

xou816 · 2024-11-12T00:33:54Z

src/api/oauth2.rs

+        .map(|s| Scope::new(s.into()))
+        .collect();
+    let (auth_url, _) = client
+        .authorize_url(CsrfToken::new_random)


should this token be validated somewhere down the line?

xou816 · 2024-11-12T00:34:12Z

src/app/components/details/release_details.rs

@@ -57,6 +57,12 @@ glib::wrapper! {
    pub struct ReleaseDetailsWindow(ObjectSubclass<imp::ReleaseDetailsWindow>) @extends gtk::Widget, libadwaita::Window, libadwaita::PreferencesWindow;
 }

+impl Default for ReleaseDetailsWindow {


thanks for taking care of clippy 😅

stevenleadbeater added 6 commits October 27, 2024 13:27

Made a start on OAuth

6497e48

Lift and shift OAuth2 flow from librespot so we can open the authoris…

919ddfd

…e URL in the default browser

format and bump version inline with librespot

ccbac9b

format and bump version inline with librespot

f68b919

fix flatpak

ef33ec9

stevenleadbeater added 2 commits October 28, 2024 12:27

fix clippy warnings

b2d77f6

fix fmt warnings

b2291e0

This comment was marked as duplicate.

Sign in to view

NatryN approved these changes Oct 31, 2024

View reviewed changes

schooldanlp6 mentioned this pull request Nov 3, 2024

Cannot Log Back In #728

Open

schooldanlp6 approved these changes Nov 3, 2024

View reviewed changes

skoruppa approved these changes Nov 7, 2024

View reviewed changes

xou816 reviewed Nov 12, 2024

View reviewed changes

AlexsandroBezerra approved these changes Nov 12, 2024

View reviewed changes

nklsfrt approved these changes Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Oauth2 login with spotify #731

Use Oauth2 login with spotify #731

stevenleadbeater commented Oct 27, 2024 •

edited

Loading

stevenleadbeater commented Oct 28, 2024

stevenleadbeater commented Oct 28, 2024

This comment was marked as duplicate.

schooldanlp6 left a comment •

edited

Loading

stevenleadbeater commented Nov 9, 2024

alumni commented Nov 10, 2024

xou816 commented Nov 10, 2024

xou816 commented Nov 12, 2024

xou816 left a comment

xou816 Nov 12, 2024

xou816 Nov 12, 2024

xou816 Nov 12, 2024

xou816 Nov 12, 2024

xou816 Nov 12, 2024

xou816 Nov 12, 2024

xou816 Nov 12, 2024

xou816 Nov 12, 2024

xou816 Nov 12, 2024

Use Oauth2 login with spotify #731

Are you sure you want to change the base?

Use Oauth2 login with spotify #731

Conversation

stevenleadbeater commented Oct 27, 2024 • edited Loading

stevenleadbeater commented Oct 28, 2024

stevenleadbeater commented Oct 28, 2024

This comment was marked as duplicate.

schooldanlp6 left a comment • edited Loading

Choose a reason for hiding this comment

stevenleadbeater commented Nov 9, 2024

alumni commented Nov 10, 2024

xou816 commented Nov 10, 2024

xou816 commented Nov 12, 2024

xou816 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenleadbeater commented Oct 27, 2024 •

edited

Loading

schooldanlp6 left a comment •

edited

Loading