Make AuthorizationCodeFlowManager support two different types of endpoints

[Ivorforce]

Hiya.

The Spotify API states often that client secrets should not be stored in the client itself. While obfuscation libraries exist, this would be merely security by obscurity.

The solution that is most often quoted is to use a proxy server which manages the actual requests to the spotify server. Some description of this can be found here. They link pre-configured backends on glitch, heroku and many more elsewhere.

I have tried this approach in a local installation and met success, by just exchanging the URLs to that of my server. Unfortunately, I don't think it's wise to maintain a fork just for this, and since it's recommended practice to go that route anyway, I made this PR with the full functionality.

In the proxy requests, no resources except code / refresh token are required.

This unfortunately turned into a larger merge request than I'd like, but I didn't see a better way to go about it. In case you feel anything is missing / needs refactor, feel free to edit. I have to note I have not tested this branch and it might fail - I only tested in my local installation with some compile hacks.

Changes

Practically, all this PR does is abstract communication endpoint information away from the code flow classes, and into "endpoint" classes built to merely communicate towards some endpoint (native or proxy). Since this information is deeply intertwined with the classes currently, it took quite the change to separate it out. Interesting classes are AuthorizationCodeFlowEndpoint and its respective subclasses AuthorizationCodeFlowEndpointNative and AuthorizationCodeFlowEndpointProxy.

The logger changes are required since static properties are not allowed in generic classes.

There is also one additional request type, which is a stripped down version of the simpler authorization token request.

[Peter-Schorn]

Thank you for this pull request. Allowing clients to select their own backends for retrieving and refreshing tokens is an excellent feature to add support for. However, this implementation seems a little over-complicated. If possible, I would also like to retain the current API by optionally allowing clients to directly provide the client id and client secret to the auth managers.

[Peter-Schorn]

It may be simpler to have the auth managers accept closures that make the requests to retrieve/refresh the tokens, in which case it wouldn't be necessary to make them generic. Although I'm still on the fence about this.

[Peter-Schorn]

It looks like there's more information about the token swap and refresh URLs here: https://developer.spotify.com/documentation/ios/guides/token-swap-and-refresh/ This information is specific to the iOS SDK, but it should be useful in determining the API for this library.

[Peter-Schorn]

I have created a branch on my repository which contains your changes. I just merged master into it and made a handful of changes myself. Please merge this into your fork.

[Ivorforce]

Hi, thanks for the quick response. I'm happy to help!

The branch you created seems to have merge conflicts with mine. Should I just accept yours?

[Peter-Schorn]

You should merge Peter-Schorn/proxy-token-server into Ivorfoce/proxy-token-server before making any further changes to the latter. I'll open a pull request.

[Ivorforce]

Right. I'm just confused as to why I'm getting merge conflicts rather than a clean merge on top of my latest commit. But I'll just reset to your branch if that's your current implementation state.

[Peter-Schorn]

But I'll just reset to your branch if that's your current implementation state.

That seems like a much simpler solution. Go ahead. The reason why you are getting merge conflicts is that I merged Peter-Schorn/master into Peter-Schorn/proxy-tokens-request. And, the revision of master that I merged is different from Ivorforce/master. You should reset both branches.

[Ivorforce]

Ok, I resolved the conflicts with master. Are there any ToDos left?

I have to once again admit I haven't tested this branch with my setup, but I don't see a reason why it wouldn't work with proxies if the native backends work fine. Or in any case I could always create a follow-up since I'd be able to make a fitting backend in a local installation built on top of this.

[Peter-Schorn]

Create a SPOTIFY_SWIFT_TESTING_CLIENT_ID and SPOTIFY_SWIFT_TESTING_CLIENT_SECRET repository secret with your client id and client secret, respectively, in your fork. Then, the github actions should be able to run. Information on how to run the full test suite can be found here.

[Ivorforce]

I see my XCode inserted tabs into your spaces files. I can probably address that if you mind. Not sure why it didn't auto-adapt to file style.

[Peter-Schorn]

I see my XCode inserted tabs into your spaces files.

That's alright. We can always worry about that later.

[Ivorforce]

Not sure what's up with the test. It's exiting with Status code 1, but after printing that all tests passed.

[Peter-Schorn]

Hmm... that's strange. Looks like I had the same issue on the latest push to master: All the tests passed but github marked the workflow as failed because the process exited with a non-zero exit code. In that case, you can consider the tests as passing. The only issue is that it seems to cause the tests on the other platform to be cancelled.

[Peter-Schorn]

you can manually re-run the workflow as described here: https://docs.github.com/en/actions/managing-workflow-runs/manually-running-a-workflow

[Ivorforce]

I tried rerunning it a bunch of times. Most times it failed with different errors, now it's back to the original (pass, but exit code 1). Ubuntu build passes completely.

[Ivorforce]

Actually, there seem to be a bunch of issues which aren't communicated in the summary. I'm not used to using Swift tests so I didn't notice this before. They seem to all be related to authorization notification calls: authorizationManagerDidChange should emit exactly once [...].

Is this the same issue that's occurring on the main branch also?

[Peter-Schorn]

Yes, I've had the same issue on the master branch as well, unfortunately.

[Ivorforce]

So - how to proceed? Any other ToDos?

[Peter-Schorn]

I'm still working on the public API and implementing tests. I haven't even started on the documentation. I'll open a pull request with my latest changes. It's not too late to suggest your own changes. In fact, I'd appreciate some feedback. My branch has, once again, diverged significantly from your branch.

[Ivorforce]

Alright, I think this PR is too large to be practically mergable.

I'll try to break it apart and build smaller PRs we can look at and merge separately. Going over the code, I'm finding these, in order of preferred chronology in merging:

1) Use AuthorizationManagerLoggers class for authorization logging. 2) Reformat use of _assertNotOnUpdateAuthInfoDispatchQueue to add to protocol 3) Rename XCTestObserver to SpotifyTestObserver and add some functionality (in case you do want this merged) 4) Pack codeflow backends into separate smaller delegation classes (reformat only; no practical changes yet) 5) Make codeflow backends generic 6) Add proxy codeflow backend

4 through 6 might be merged if I notice they're small enough.

Let me know if you have any comments. I'll get started on 1 and 2 right away since I think they're no-brainers.

[Peter-Schorn]

Have you made any significant changes to your branch since it was last synced to mine? If not, you may want to force reset to my branch. It would be much simpler.

[Ivorforce]

I have pushed the two easily separable parts and commented some reasoning for it. Once again, I'd strongly advise merging in small chunks were possible, especially for reformat-only PRs.

I will continue soon by looking over your recent changes and seeing if I have any comments.