Authentication refactor

[jcbirdwell]

This is the implementation of #487. It also fixes some issues I ran across while writing the tests for my proposed feature that were preventing a full pass. I can open issues for each or all together if needed, just let me know. Otherwise, they were:

1. A test was throwing a depreciation warning stemming from a regex search in get_album_browse_id, I changed it to use a str.find slicer, which fixed the warning and made it a touch faster.
2. It looks like the songs section of charts is no longer provided by YouTube (could be regional), which was producing an indexing error in get_charts when authenticated. I changed it to check that the results array had enough members before it inserted the songs key. This prevents the error without depreciating the feature should the section be added back on googles end.
3. While prepping a YouTube music account library for the tests I couldn't find the method for saving albums, so I wrote one, only to find afterwards it was written in as a footnote of the rate_playlists method. I went ahead and committed it as an alias/convenience method for other idiots like myself that couldn't find the other. Ignore it if you want.
4. I may have just been cluelessly butchering my test.py and test.cfg trying to get unitests up and running, but there were a few variables that seemed to be library or region dependent, which I externalized to the cfg so as to be more clearly customizable. If that was a bad call and the purpose of the tests went over my head, let me know so I can redo and rerun with the other variables.

[sigma67]

Hi @jcbirdwell , thanks so much for contributing this back. Glad to accept this contribution with a few changes.

Please also add documentation in the Setup section (new chapter in Oauth section). How could one obtain such custom credentials?

Requesting a review from @MarvinSchenkel who contributed #396

I will wait for these changes before running the tests myself, I assume you have done so on your machine

[sigma67]

I suppose the previous check was too simple since you'd actually need to check for a premium subscriber instead of a just a logged-in user. Should probably add a separate account to the tests that is authenticated but not subscribed

So I think we can just remove the bool(self.auth) part altogether since it does not actually make a difference

[jcbirdwell]

I was indeed performing all tests using non-premium accounts.

[sigma67]

So we can remove bool(self.auth) and ?

[heisen273]

Tested this PR and I think all requests through mixin's are broken._.

at first, __init__() goes OK and i can obtain refresh token with my custom client id & secret pair:

But then, when i do any request(.get_me(), .get_playlist(), etc) - it always returns 401:

---

I think you have to update _send_request() with logics that handles if self.alt_oauth: ...

[jcbirdwell]

I ended up going with a mix of my original issue solution alternative 2 and the dataclass implementation mentioned previously. While I was adding a credential dataclass I was reminded of why I was originally trying to pass the YTMusicOAuth instance from YTMusic, it was to prevent recreating the OAuth class on every single request. I believe the original intent was to ensure that the authorization field of the header was always up-to-date, this version accomplishes that but instead of rebuilding all the headers each time it just does authorization/ones that update.

It also catches #492 in the crossfire and I believe the issue @heisen273 was having with mixins, but I couldn't replicate it so I'm unsure. It's a decent chunk of changes, but should act as a drop-in replacement.

I added custom oauth handling and believe it maintains compatibility with #396, but I don't think there is a test for it.

I haven't written any of the docs or additional feature tests yet as I wanted to get feedback before tackling em, but I'm getting a full pass for existing tests. In light of that, I'd convert the pull request to a draft, but I'm not positive what that does and don't want to mess anything up.

[sigma67]

I'm not sure this second changeset was needed based on what you wanted to achieve. But I like the improvements in general, so let's try to make it work.

I have a very hard time verifying that everything is still working, considering that 18 tests are breaking for me locally even after updating the test.cfg (those changes are quite welcome, thanks).

I suspect most failures have the same origin, some sort of (breaking?) change to the local oauth.json file that I have on my machine. I have attached my test log, maybe you can point out what's wrong.
tests.log

I will also state here that this MR should be free of any breaking changes to the public facing API.

Don't worry about draft mode, it has no effect in this repo (since we're currently not running tests on PRs due to somewhat sensitive nature of the credentials)

[sigma67]

The error message has changed slightly, but I'm still getting this:

======================================================================
ERROR: test_search_library (test.TestYTMusic.test_search_library)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "\ytmusicapi\tests\test.py", line 164, in test_search_library
    results = self.yt_oauth.search(config['queries']['library_any'], scope="library")
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "\ytmusicapi\ytmusicapi\mixins\search.py", line 165, in search
    response = self._send_request(endpoint, body)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "\ytmusicapi\ytmusicapi\ytmusic.py", line 200, in _send_request
    if 'X-Goog-Visitor-Id' not in self.headers:
                                  ^^^^^^^^^^^^
  File "\ytmusicapi\ytmusicapi\ytmusic.py", line 191, in headers
    self._headers['authorization'] = self._token.as_auth()
                                     ^^^^^^^^^^^^^^^^^^^^^
  File "\ytmusicapi\ytmusicapi\auth\oauth.py", line 85, in as_auth
    return f'{self.token_type} {self.access_token}'
                                ^^^^^^^^^^^^^^^^^
  File "\ytmusicapi\ytmusicapi\auth\oauth.py", line 178, in access_token
    self.token = self.credentials.refresh_token(self.token.refresh_token)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "\ytmusicapi\ytmusicapi\auth\oauth.py", line 141, in refresh_token
    return OAuthToken.from_response(response)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "\ytmusicapi\ytmusicapi\auth\oauth.py", line 30, in from_response
    return cls(**data)
           ^^^^^^^^^^^
TypeError: OAuthToken.__init__() missing 1 required positional argument: 'refresh_token'

[sigma67]

Sorry for all the comments, I'd consider this to be the final review hopefully.

Tests are passing for me.

This one's come a long way and I think it's in a great state now.

Really need to add a proper linter like ruff and some sort of basic no-oauth PR pipeline...

[sigma67]

So we can remove bool(self.auth) and ?

[sigma67]

l.51 through l. 73 can be removed as they are not used anywhere, unless I'm missing something

Perhaps a leftover from the refactor

[sigma67]

this has not been resolved

[sigma67]

Not related to this line, but please add/update documentation in docs/source/setup

[jcbirdwell]

Sorry for all the comments, I'd consider this to be the final review hopefully.

I really appreciate all the continued feedback on this PR and I'm satisfied with the direction we indeed up taking as a result. I'll be busy for the next few days, but it shouldn't take long to wrap up the final stuff when I can get back on it.

[sigma67]

Thanks for these changes again. It would be great if you could turn all the comments into proper docstrings (most of the comments I added are just todo items regarding this):

https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#directive-autoproperty

See the class example here for properly documenting instance variables.

You can use either style - just add a colon to the current comments or place them in the line above with #: (the latter is my preferred style)

Other than that there is only one finding left regarding a try-catch you added

[sigma67]

Hi @jcbirdwell , I fixed the remaining issues & merged. Feel free to add the documentation in a separate PR