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

OAuth doesn't work with arbitrary file names #376

Closed
benblank opened this issue Apr 10, 2023 · 3 comments
Closed

OAuth doesn't work with arbitrary file names #376

benblank opened this issue Apr 10, 2023 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@benblank
Copy link

Describe the bug

When creating a YTMusic instance with OAuth credentials from a file not named "oauth.json" (or rather, the provided path to which does not contain the string "oauth.json"), the resulting instance cannot successfully connect to YouTube Music. Worse, the resulting error (a complaint from the requests library that a header is of the wrong type) doesn't provide any information which helps identify the actual problem (the authentication data being interpreted as invalid browser auth) unless one reads the ytmusicapi source.

To Reproduce

Steps to reproduce the error:

  1. Make and enter a new, empty directory.

  2. python3 -m venv .venv

  3. . .venv/bin/activate

  4. pip3 install ytmusicapi

  5. ytmusicapi oauth

  6. Follow the instructions as usual to create "oauth.json".

  7. mv oauth.json oauth-foo.json ← And here's the problem, folks. 🙂

  8. Create a new file "example.py" with the following contents:

    from ytmusicapi import YTMusic
    
    print(YTMusic("oauth-foo.json").get_playlist("LM", limit=None))
  9. python3 example.py

  10. Observe the resulting InvalidHeader exception complaining that the value of the "expires_in" header should be a string or bytes object.

Additional context

I ran into this because I'm trying to migrate some data between two accounts, so naively renamed the files created by ytmusicapi oauth to "oauth-.json".

Possible solution

(Note that I'd be happy to submit a PR for this; I'd create it now, but I need to step away from the keyboard for a bit.)

Looking at the source of the prepare_headers function, I see that it's detecting whether to use OAuth by examining the file's name; I think it would be less surprising to base it on the contents, instead.

This could be done, for example, by checking for the presence of a key which is required to be present when using OAuth but is unlikely to be found in the headers used for browser authentication or by checking whether "expires_at" or "expires_in" keys are present and have have an integer value, as integers aren't valid header values.

Alternatively, input_json could simply be assumed to contain OAuth data (optimistically trying to create a YTMusicOAuth instance), with a fallback to browser authentication if a KeyError is thrown. YTMusicOAuth.load_headers appears to require that the keys "access_token", "expires_at", "refresh_token", and "token_type" all be present; it seems very unlikely that all four would be found in a browser's request headers, as none of them are standard HTTP headers.

@sigma67
Copy link
Owner

sigma67 commented Apr 10, 2023

Yes, I didn't really see a probably with mandating a specific filename. But I suppose it's more flexible to allow any filename, as it's being passed in through the constructor.

Regarding an implementation, I think we should implement some validation for the files. For browser-based auth, we can rely on the presence of the Cookie value, while for oauth the following are mandatory: token_type, access_token, refresh_token and expires_at.

@sigma67 sigma67 added the enhancement New feature or request label Apr 10, 2023
@sigma67
Copy link
Owner

sigma67 commented Apr 10, 2023

For validation, create functions in auth/browser.py and auth/oauth.py to separate that logic from the ytmusic.py file.

For browser validation we should reuse the existing logic:

auth_header = self.headers.get("authorization")
self.is_browser_auth = auth_header and "SAPISIDHASH" in auth_header
if self.is_browser_auth:
try:
cookie = self.headers.get('cookie')
self.sapisid = sapisid_from_cookie(cookie)
except KeyError:
raise Exception("Your cookie is missing the required value __Secure-3PAPISID")

@sigma67 sigma67 assigned sigma67 and unassigned benblank Apr 19, 2023
@benblank
Copy link
Author

Yes! Sorry, I've had a bit less time than expected, but I'm very much working on it.

Basically, I was finding it awkward to track the different types of authentication (none, browser, OAuth) and handle them consistently with the logic being spread out, so I'm doing some minor refactoring to centralize it for each type. I'll try to get a draft PR up later today with an example of what I'm talking about, so that you can actually visualize it. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants