Restructure the authentication process

[marioortizmanero]

Back when I first rewrote the authentication process I did improve a few things I thought were necessary, like only having a single http client per Spotify instance, and being more ergonomic and easy to use.

But it still leaves a few things to be desired. For example, an InvalidAuth error variant is needed for whenever a user calls a method from Spotify that isn't available due to the authentication process. If one were to use the Client Credentials Flow, the endpoints with access to the user's data shouldn't be available (say me or currently_playing_track). You need to use the Authentication Code Flow for that. This worked well enough because we only had two different methods: the Client Credentials and the Authentication Code.

Now that the PKCE Code Authentication Flow has to be implemented, I think it's time to re-think the architecture we're following and make it more idiomatic. This auth flow can access the same endpoints the regular Code Authentication Flow can, but it requires a different setup.

We mainly need three levels for this problem:

• The HTTP client to actually make the requests, which already exists as rspotify::http::BaseClient. I'll call it HTTPClient from now on.
• BaseClient, a base Spotify client, with access to endpoints such as tracks or search that don't need access to the user's data. This uses HTTPClient to make requests.
• UserClient, an user authenticated Spotify client, with access to endpoints such as me. This may inherit from BaseClient and implement the extra endpoints over it, or use HTTPClient directly and only have the user-authenticated ones.

Rust doesn't really have OOP, but we can work it out with traits. Here's the first solution, where the UserClient inherits from BaseClient so that the Code Auth flows can access both UserClient and BaseClient methods:

And here's another solution with a more horizontal hierarchy. UserClient only includes the user authenticated methods because it doesn't inherit from BaseClient, so the Code Auth flows have to implement both of them. I feel like this is more natural in Rust, and it seems more modular because it doesn't assume that user authenticated flows can access the regular ones.

In both of these models the implementations take place in the traits, which only needs access to some HTTPClient methods, so the structs would only need to implement at most a couple getters for their internal fields, kinda like how Iterator works.

/// Just a rough idea of how it would look like to use the proposed models.
fn main() {
    // This is the same
    let credentials = CredentialsBuilder::from_env().build().unwrap();

    let spotify1 = CodeCredentialsFlow::default()
        .credentials(credentials)
        .build()
        .unwrap();

    // This wouldn't work, as `currently_playing_track` is not a member of
    // `CodeCredentialsFlow` (which only implements `BaseClient`).
    // spotify1.currently_playing_track();

    // This is the same
    let oauth = OAuthBuilder::from_env()
        .scope("user-read-playback-state")
        .build()
        .unwrap();

    let spotify2 = AuthCodeFlow::default()
        .credentials(credentials)
        .oauth(oauth)
        .build()
        .unwrap();

    let spotify3 = PKCEAuthCodeFlow::default()
        .credentials(credentials)
        .oauth(oauth)
        .build()
        .unwrap();
}

/// Now that we know what flow is being followed, we could also avoid
/// `derive_builder` so that we don't need `.build().unwrap()`.
fn main() {
    let spotify = CodeCredentialsFlow::new(credentials).unwrap();

    let spotify = AuthCodeFlow::new(credentials, oauth).unwrap();

    let spotify = PKCEAuthCodeFlow::new(credentials, oauth).unwrap();
}

This is purely theoretical so I'm not sure how easy it is to implement. I'll try to code a rough sketch of the latter model, although I'll be quite busy soon with finals so I'm not sure when that'll be possible.

What do you think? Any more ideas?

[ramsayleung]

Rust doesn't really have OOP, but we can work it out with traits.

Yep, but in this case, I think traits can something like interface in Java.

I personally prefer to the second proposal, since it has horizontal hierarchy than the first proposal.

As for the idea that dispatching different endpoints to different authentication clients based on its authentication requirement and flow, it's a great point. But IMHO it will make the endpoints a little bit difficult to use and become less ergonomic, since the developers have to know that does this endpoint need to authenticate with Code Auth Flow or not?

As for other solutions, I am a bit busy on my job, I would propose my thought later, perhaps on this weekend.

[marioortizmanero]

As for the idea that dispatching different endpoints to different authentication clients based on its authentication requirement and flow, it's a great point. But IMHO it will make the endpoints a little bit difficult to use and become less ergonomic, since the developers have to know that does this endpoint need to authenticate with Code Auth Flow or not?

How is it less ergonomic? The issue is that users may try to access methods that shouldn't be available. The difference is that rather than allowing them to access these methods and get an authentication error from Spotify, they'll find a method not available error at compile-time. Not sure if I understood your comment well. Ah ok I did misunderstand the comment. You can usually tell if it needs user authentication in the docs page, or by the name (user_follow_artist obviously requires access to user data).

What I recognize may make it less ergonomic is the fact that the users will have to use Trait when they try to call the methods. But that can be fixed by having a rspotify::prelude module with all the traits in the library. That way use rspotify::prelude::* will fix any of these issues, and it's a common pattern in other libraries and even std (see std::io::prelude).

[marioortizmanero]

I've been experimenting with this a bit, here's how it would look like more or less: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=e5192749c928122c465d3aa18ede67ce

[marioortizmanero]

And I've made a post on the official Rust subreddit to get some advice regarding Rspotify's architecture, see the discussion here: https://www.reddit.com/r/rust/comments/lkdw6o/designing_a_new_architecture_for_rspotify_based/?

[oldwomanjosiah]

I've been tinkering with a library for the Twitch Helix API and have been running into similar issues. Would be very interested to see what you come up with.

[marioortizmanero]

Note: this will be modified after #173 is merged, since the HTTP request methods are no longer implemented over the Spotify client. Instead, the HTTP client is a different struct to keep the logic separate and the abstraction over multiple http libraries clean.

[marioortizmanero]

Here's a draft of how it would look like with the proposed changes https://github.com/marioortizmanero/rspotify-architecture, can you take a look before we modify the client anymore, @ramsayleung? Thanks!

The final architecture is still based on the trait system, with a few tweaks to incude HTTPClient as a member instead of "inheriting" from it. It can get repetitive at times (implementing BaseClient and OAuthClient and similars), so i could use a very simple macro to reduce that. But even then it's not that much boilerplate compared to how much flexibility it offers.

I'd like to get this done after #161 to avoid possible conflicts.

[ramsayleung]

The full trait inheritance is a little bit complex:

Let's take a look at the simplied one:

The new architecture is much clear than the current one, since the endpoint flow and auth flow are coupling tightly. There is one question about this architecture proposal, since the CodeAuthPKCESpotify and CodeAuthSpotify need to implement four traits at once, it seems it's unnecessary. Is it possible to combine BaseEndpoints into BaseClient, it make sense to combine the unidirectional trait inheritance into single one, right? The the trait inheritance will become:

[marioortizmanero]

The new architecture is much clear than the current one, since the endpoint flow and auth flow are coupling tightly. There is one question about this architecture proposal, since the CodeAuthPKCESpotify and CodeAuthSpotify need to implement four traits at once, it seems it's unnecessary. Is it possible to combine BaseEndpoints into BaseClient, it make sense to combine the unidirectional trait inheritance into single one, right? The the trait inheritance will become:

Yes, I asked that in the original comment and you agreed with me that it'd be better to have a horizontal hierarchy:

I personally prefer to the second proposal, since it has horizontal hierarchy than the first proposal.

But it might be unnecessarily complex for now. I don't think there'll be an auth method that will let you access user methods but not the basic ones, so a vertical hierarchy makes sense to me. While we're at it we can merge OAuthClient and OAuthEndpoints into a single OAuthClient as well, which is the same as merging BaseClient with BaseEndpoints.

Can you create a diagram with the full hierarchy once the architecture is definitive so that we can include it in the docs for newcomers? Do you agree with everything else? Maybe other contributors like @kstep or @martinmake can provide their opinions.

[ramsayleung]

While we're at it we can merge OAuthClient and OAuthEndpoints into a single OAuthClient as well, which is the same as merging BaseClient with BaseEndpoints

It makes sense.

Can you create a diagram with the full hierarchy once the architecture is definitive so that we can include it in the docs for newcomers?

Yep, great idea, it will be a pleasure to do that.

Do you agree with everything else?

Yeah, your proposal is complete, it seems there is nothing left to discuss.

[marioortizmanero]

Last thing to consider before I start working on this small rewrite:

Now that the main client doesn't need the builder pattern for its initialization, should we completely get rid of it? Other structs like Token don't really need builder patterns, we were using them for consistency, really. Instead of:

let tok = TokenBuilder::default()
    .access_token("test-access_token")
    .expires_in(Duration::seconds(3600))
    .expires_at(now)
    .scope(scope.clone())
    .refresh_token("...")
    .build()
    .unwrap();

You can just write:

let tok = Token {
    access_token: "test-access_token".to_owned(),
    expires_in: Duration::seconds(3600),
    expires_at: now,
    scope: scope.clone(),
    refresh_token: "...".to_owned(),
    ..Default::default()
};

We could get rid of derive_builder that way. The last way of initialization with Default::default is a bit less known, to be fair, so that's why I'm asking first -- although we could add an example in the docs.

All structs currently using the builder pattern:

• Token: not that much usage, could work with the typical struct initialization.
• OAuth: also commonly used and has 4 fields so a new function would look worse.
• Credentials: only has two fields and is commonly needed, so we could implement a new function.

The TokenBuilder::from_cache would have to be Token::from_cache now, returning Option<Token>. Same goes for CredentialsBuilder's and OAuthBuilder's from_env.

Feel free to deny this proposal as I understand it might not be necessary, I just want to at least mention it before this is implemented, now that it's possible.