Lautaro-Garcia / counsel-spotify

Control Spotify App through Emacs
GNU General Public License v3.0
59 stars 8 forks source link

Using oauth - suggestions and example implementation (WIP) #11

Closed louixs closed 3 years ago

louixs commented 4 years ago

Hi @Lautaro-Garcia!

It's me again.

I've been thinking about extending this package to use oauth so we can choose from wider selection of Spotify API. For example, I found out that shows and episodes search wasn't possible with access token. Another example would be to have access to user playlists rather than the generic search across all Spotify playlists (i.e. my earlier commit I sent for pull request). I've included all my example above in this preliminary work.

This is still not ready yet but I wanted to see what you think of the idea of using oauth.

There are also a few caveats if we completely switch from access token to oauth. Oauth2 package which I'm using here doesn't seem to handle token refresh well. So we may have to implement refresh logic ourselves. Oauth2 package stores tokens using plstore package which caused some issues on my machine when setting up gpg. Basically this can cause annoyances among the users who are happy with what this package already provides with access token. So one thought that occurred to me is to keep the usage of access token until user wants to use APIs that require Oauth tokens.

I hope you are staying safe in this difficult time. And it would be great to have some of your thoughts on this! (Just to make sure this is still work in progress)

Many thanks!

Lautaro-Garcia commented 4 years ago

Hello there!

I think that is a great idea! I would love to access as much content as possible through counsel-spotify. I think the same way as you do, it may be a little bit harsh to make the users configure plstore in order to use this library, so I think I would keep both kind of authentication in parallel. Do you know if there's another alternative way of doing oauth in elisp, or if another package that you know of does this kind of stuff? I would like to compare solutions provided by other Emacs packages to see if they found a way to do this kind of auth in a smooth way. I don't particularly like to force the users to use a third-party tool, but I would consider it if there's no other way.

I want to read a little bit about Spotify auth, it's been a long time since I last checked out their docs :sweat:

Thank you for doing this work, I think it's pretty neat, and will allow us to do a lot of cool things!

louixs commented 4 years ago

Good to hear we're on the same page in terms expanding features with oauth! I'd agree about keeping both kind of auth in parallel.

To your question about different ways of doing oauth in elisp, unfortunately I haven't come across any yet or happen to know about it. I was looking at another Spotify package for emacs (https://github.com/danielfm/spotify.el) but it seems to be using the same oauth2 package with some of their own modification. From what I gathered after a quick scan, users of that package would still be using plstore.

Looking at existing solutions and comparing them sounds like a good approach to me as well. I'll keep looking. Many APIs require oauth nowadays so there must be another package that had to deal with oauth.

Thankfully, Spotify has a really good documentation about their API including the auth flows. I guess you've probably seen this anyways but if it helps: https://developer.spotify.com/documentation/general/guides/authorization-guide/

I'll keep you posted about my findings!

louixs commented 4 years ago

Hi @Lautaro-Garcia!

I've finally got back to have a look at this. Hope you're doing well.

I looked around for emacs packages that uses OAuth to compare as we discussed. My finding so far is that there doesn't seem to be a standard way of handling OAuth 2 in a simple and easy manner.

To make my finding more concrete, I have taken a stab at making a package to handle OAuth 2 to see if it makes it easier for us. Also I think OAuth 2 flow should ideally be completely decoupled from the packages that uses it. This is another reason why I went all the way to make a separate package. And I've just pushed it to the oauth branch here: https://github.com/louixs/counsel-spotify/tree/oauth Since it's quite basic, I haven't published it elsewhere. It currently lives here https://github.com/louixs/elisp-oauth-2 as well as in my forked counsel-spotify oauth branch.

The main idea is to make it up to the users to decide how and where to store their tokens just like you have done for client id and client secret. For OAuth 2, we obtain refresh token once user has done authorisation. We can then keep using refresh token to keep refreshing token so that user doesn't have to authorise again. I have made it so that the package reads refresh token from global variable. User can put it in init.el to make sure that it's set before it's used. This way, user can choose where and how to store refresh token once user has done authorisation.

The main difference from the current counsel spotify is that it calls init-oauth-2 functions once when it loads to set the necessary variables. And there are two example interactive functions that uses the new oauth 2 package. For example this function counsel-spotify-search-show-2 uses the show search api that requires oauth 2. When you run this for the first time, it should open a browser to ask for authorisation. It also runs a local web server for redirect (on port 8080 at the moment - probably should make this variable), so once authorised sptofiy redirects to localhost:8080 where the token is displayed for user to copy. When user goes back to Emacs after copying, Emacs prompting user to paste the token. Once user pastes the token, we then issue a request to get access token and refresh token. Finally if this succeeds it also shows a message for user to save refresh token to init file. So that user only has to do this authorise flow only once.

The idea is to only use this OAuth 2 requests for the search/api calls that needs OAuth 2. And leave the searches that don't require OAuth 2 as is. Please note that this is still work in progress.

I've formed this idea after looking at these packages below.

1. Another emacs Spotify client: https://github.com/danielfm/spotify.el
Uses oauth2 which we'd like to avoid because it forces plstore. Oauth2 also seems to have issues refreshing token so

2. Reddit client for emacs: https://github.com/ahungry/md4rd I've taken a lot of ideas from this package for storing, and refreshing tokens. I've also discovered that there's a package called request.el for handling http calls. To me it's a lot easier to use than url.el. So for my oauth-2 example I've pushed up you'll probably notice that I'm using request.el. I've also discovered another great library called dash.el that is a bit like underscore.js if you're familiar with Javascript. I'm using some of its functions to make things simpler.

3. Emacs client for slack: https://github.com/yuya373/emacs-slack I had a brief look at this massive package. It's so huge that I've had a little difficulties to figure out how it uses oauth2. But it seems to be a well written one so it might be worth digging into it deeper. Otherwise, in its readme it mentions auth-source Emacs package that allows users to securely store tokens or other secrets. I personally didn't know about this so it's a great information. But again, I think we don't want to enforce our users to use this kind of thing. So didn't end up using it.


This ended up being a super long essay! I hope it was understandable. Let me know if anything is unclear. I'd be curious to hear if you've found anything else or if you have different ideas!

Lautaro-Garcia commented 4 years ago

Hello, glad to hear from you!

Sorry, I haven't had enough time to look at this with the attention it requires, but I think that's an excellent work!

I really like the idea of having the oauth client in a separate package, I think it's a fair separation of concerns. Sorry I haven't checked out the implementation, but I was thinking the same way about having the two auth mechanisms available and using the one required for the particular action the user wants to do.

One thing I did check out was plstore, and I think I changed my mind, or didn't understand you correctly the first time. It's a package currently bundled in Emacs, so we wouldn't be forcing our users to set it up in any way, I think (though I'm not entirely sure, I have to take a deeper look on its API).

I'll try to make some time to look at this, this quarentine has me super busy! Thank you very much, amazing job!

louixs commented 4 years ago

Hi!

No worries and thanks very much for your feedback and ideas! I found some issues with the separate ouath package. But I think it makes working on the actual counsel spotify functionalities a lot easier. I'll try to get it to a somewhat stable point.

You are 100% right about plstore that it ships with Emacs. Sorry I was a bit unclear I think. I'll also need to educate myself more on how it works to be honest. But after working on the oauth package for a bit so far, I looks like the only token that needs to persist, aside fro the client-id and client-secret, would be refresh token. So what I'm thinking now is that perhaps we ask users to set refresh token like client-id and client-secret mentioned in the readme.

This way we can keep how the tokens are stored decoupled as well. Of course we can use plstore to make it easier for users. I can imagine there may be many different ways to store and set variables for emacs depending on users' preferences and system environments. So keeping it fairly generic might make our lives easier as the ones to maintain packages..

For example, I store my Spotify credentials and refresh token in environment variables file for my shell. And I read those and set to counsel spotify variables in my emacs init files.

Please let me know if I'm not clear on any of those above or if you disagree :)

Following up on the idea about keeping two auth mechanism, I'll have to work on that as well. To make it easier, in my branch I'm using only one type of auth. I'll let you know once this is done as well.

Same here with this current quarantine situation. I've been fairly busy as well. I hope you are well!

louixs commented 4 years ago

Btw, I've started testing out adding new functionalities like displaying currently playing song which is part Spotfiy's Player API. Now that we have the oauth authorization tokens, we can use the full range of APIs that spotify offer. It's quite nice :)

I'm thinking about playing around a bit more with the player API. The next thing I'm trying to do is to add the currently playing song to favourite or to a playlist.

Lautaro-Garcia commented 4 years ago

Yes! I always wanted to add the feature to add the current playing song to a playlist or enqueue songs!

I wanted to let you know that I'm currently playing a little bit with automated testing of all of this (the things that are possible, mocking out Spotify, I don't know if there is a mock server that could be used nor if I want to waste too much time on this) seeing that this little package can grow a lot. I'll try to push the branch that I'm working on as soon as I can, but bear in mind that I'm trying to split the single file so every current "section" gets its own file, I think it will be less scary to modify that way.

I don't have any hurry though, so I could wait until your PR gets merged or help you resolve conflicts if that were the case.

Thank you!

louixs commented 4 years ago

Hi! That's great to hear that you've also wanted to add songs to a playlist or enqueue them! Actually enqueueing would be a really handy feature. I'll see if I can get it working soon. I can imagine that adding songs to a playlist will be a little more involved so I might do it at a later stage.

Thanks so much for letting me know about your work about testing! It would be no doubt great to have test coverages as this package grows! I've also had some occasional thoughts about adding tests so I'll look very much forward to seeing your branch!

And great idea on splitting the package into smaller files! 100% on the same page. Don't worry about the merge conflicts and carry on with what you are doing. Once you have your branch up, I can try to have a look and resolve them. I'll let you know if I need any help!

Thanks!

Lautaro-Garcia commented 4 years ago

Hello! I finally had enough time to sit and review your code, read Spotify documentation, and read a little bit of the Emacs implementation of the oauth authorization code flow. Here's what I think:

  1. Sorry I didn't had the time to see this before, but I think I could be a good idea to use the oauth2 package, but just for the sake of not re-implementing all the ouath logic by ourselves, and have less things to maintain in the future. I was thinking about storing the refresh token in a variable, like the client-id is stored nowadays. I'm on the fence about that, because although they are long-lived tokens, when they expire the user will have to update their config. The thing that scared me at a first sight were the lines added to the README file, where you stated that there were some configuration needed in order to make that work. Do you know if it's optional? Can we use plstore and not set up those variables?
  2. I saw a couple of -builder and -format and make-query updates in order to support the new "Spotify objects" (like show, playlist, and such). I think in the branch I told you about they are some changes that address this issue: now it isn't mandatory to define formatters or "builders" (I called the function "parser" in the sense that it parses Spotify's response in order to make a counsel-spotify object) for objects that only have a name. They are the default object and use the default implementation (it isn't even required to "categorize" them. You could use a playlist type of object without defining a counsel-spotify-playlist class)
  3. I keep thinking about the dual authentication. In an ideal world I would like to not separate them per endpoint, because as I see it (correct me if I'm mistaken) the new auth is a superset of the old one: if I have that access token I can use all the previous endpoints plus a bunch of new ones. So I think I would like that the -search-* commands worked a little bit like this (I'm thinking out loud here, I don't know exactly how to implement this):
    • If the user has the new auth set up, use that
    • If they don't, but have the client-id and client-secret, try those
    • If everything else fails, we're sure they don't have access to that particular API. We show the user a message saying so Another option is to use some generic functions that specialize on the auth type. At load time, we already know what kind of authentication the user has set up, so it may not be necessary to do the flow I mentioned before. We could reify the auth types, save them in a variable and do something similar to what the package does with the Spotify backends (that are based on the OS, and are resolved at load time). We could call those functions with the Spotify API that we want to use (like search API, or Episodes API) and the auth type and it would know if it has enough permissions to ask for that thing. I don't know, I'm just shouting out ideas here :grin:
  4. The branch I was working on is quite finished (is this one). I'm reading a little bit about how MELPA bundles things, so I don't break the package in the next release :see_no_evil: . in that branch I split the main file, made some refactors (the ones mentioned in the previous points) and added tests and a Github Action as a form of CI.

Ok, that was a huge wall of text, let me know what you think! Stay safe!

louixs commented 4 years ago

Hi!

Thanks so much for spending time with your review and your feedback!

  1. I'd agree with using the existing oauth2 package. You raised a good point about not reinventing the wheel so to speak and having to maintain another package. Storing refresh token in a variable to me also seems reasonable but I think your suspicion is right, after doing some searching it looks like refresh token can expire as well. So having user save their refresh token will cause inconveniences when refresh tokens do expire. About the plstore config I have put in the readme, I think it was more for a note to myself while I was looking at oauth2 package. Sorry for leaking that there. It's been a while since I looked at oauth2 that uses plstore to store tokens, I still vaguely remember that there plstore needed your gpg to be configured and it wasn't very straight forward. Having said that though, to tie this section up as well, may I suggest that I take another look at oauth2?

  2. Thanks for the heads up about the changes. My plan would be to rebase my changes onto your new changes. I see that it's merged to master now as well so let me have a look at that to understand the new structure. I really like that the files are clearly separated. Also the tests look great as well. Thanks for adding those!

  3. To your first point about the new auth being superset, you are correct! So in an ideal world, we just ask users to set it up once (provided that the refresh tokens are also automatically handled for the user) and that should be the only time user have to see a web browser launching confirming users of the permissions/scopes this package would need. But I guess in the real world that we live in, as you alluded to, we may have to go with some form of dual token if we want to keep it as trouble-free as possible for the users. As for your ideas, I think both of them are possible. For example, if refresh token already exists the user has the "superset" access token besides client-id and client-secret (Authorization Code Flow). This basically gives access to scoped data. If that's not the case and user has only client-id and client-secret only then user can use access token with (Client Credentials Flow). This gives unscoped data which is what's currently available with this package. I'm thinking that your first idea might actually be the easiest way given that we can just use the superset token if the user has one or obtains one. If the user would like to use any function that asks for the scoped data, user will have to obtain the "superset" token via the web interface the first time (The step one shown in the diagram here). Another thing is that we the knowledge of which endpoints are unscoped. So we can let user just use the package without prompting to obtain the scoped token (superset token) until user calls a function that requires scoped token. The downside of this approach would be that there's always a chance that the set of unscoped endpoitns change and we have to attach some data for each endpoint or function to indicate whether they are scoped or not.

  4. Thanks again for the heads up about your branch. I'll have a look.

Lastly, as I indicated above what I would suggest to take this forward would be as following. I'll look at your new branch (now new commits on master), and will rebase my work on top of your latest work. I'll use oauth2 package. And I'll also try to incrementally add new feature onto the new work so it's easier to review. (I ended up piling up a few things on the current branch so it's unclear what's been added.)

I'll get back to you once I have a working implementation using oauth2 and a function that uses a scoped endpoint.

Mine also ended up being a long essay again!! Again, if anything is unclear feel free to let me know! Thanks!

louixs commented 4 years ago

By the way, I remembered the reason why I moved away from oauth2. There were a couple of issues with the library itself. Among the issues are broken auto refreshing of access token, some complexity and inconvenience with how plstore needs to be set up (we've discussed this before briefly). It looks like there are others like us wanting to use oauth2 for their emacs package but have come across similar issue.

I found this one for example discussing issues and potential solutions around the oauth2 package. https://github.com/kidd/org-gcal.el/issues/79

This seems to be a bit more complicated than what I had remembered to be. I'll have look around a bit more to see what are the alternatives...