[WIP] Auth restructure #207

marioortizmanero · 2021-04-29T19:12:45Z

TODO:

Note that this only really separates the rspotify-http crate, as I don't think moving the auth logic to a separate crate is really worth it. We can discuss it but I'm not sure it's necessary at all.

Closes #207
Closes #208

marioortizmanero · 2021-04-29T23:21:55Z

Ok so turns out this trait-based implementation is only possible if we use -> Box<dyn Paginator> instead of -> impl Paginator.

This means that we are forced to use Box + dyn if we want it to work, for now. This is sub-optimal because there's a slight runtime overhead in comparison to generics, but on the other hand there should be less code bloat due to generics.

We already do something similar for every endpoint anyway because of maybe_async, which may add #[async_trait] to our HTTP client trait when an async client is configured. This proc macro does the same thing but for Future return types as a workaround until rust-lang/rust#63066 is implemented and stabilized.

This whole thing could also be fixed with delegation instead of a trait system, but that's not happening anytime soon, so I would keep going with traits for now: rust-lang/rfcs#2393 (comment).

marioortizmanero · 2021-04-30T13:12:00Z

Unfortunately the diff is going to be a mess because I'm splitting client.rs into different files and git isn't able to keep track of that. So I would appreciate no breaking/huge changes to the library while I'm working on this, if possible.

marioortizmanero · 2021-04-30T20:49:58Z

So I've got a couple examples working now with the new interface. You can take a look and let me know your thoughts/suggestions:

* https://github.com/ramsayleung/rspotify/blob/auth-rewrite/examples/code_auth.rs
* https://github.com/ramsayleung/rspotify/blob/auth-rewrite/examples/client_credentials.rs

Actually all of them work now, except for the PKCE one which is still WIP. But I might leave the PKCE implementation for another PR to avoid changing too many things in this huge one.

ramsayleung · 2021-05-13T13:39:01Z

Cargo.toml

@@ -25,19 +26,23 @@ resolver = "2"
 [dependencies]
 rspotify-macros = { path = "rspotify-macros", version = "0.10.0" }
 rspotify-model = { path = "rspotify-model", version = "0.10.0" }
+rspotify-http = { path = "rspotify-http", version = "0.10.0", default-features = false }


Why doesn't rspotify-http enable by default?

Because we need to manually specify which client to use. Perhaps rspotify-http shouldn't enable clients by default, and that should be done by rspotify itself?

Cargo.toml

rspotify-http/Cargo.toml

ramsayleung · 2021-05-13T13:58:40Z

src/client_creds.rs

+
+        self.token = Some(self.fetch_access_token(&data).await?);
+
+        self.write_token_cache()


Should we implement cache token feature in this PR or leave a TODO item?

I did implement cached tokens in this PR, but I've left refreshing ones for another. But you can just ignore them for now and review it later on.

ramsayleung · 2021-05-13T13:59:19Z

src/client_creds.rs

+
+    /// Obtains the client access token for the app. The resulting token will be
+    /// saved internally.
+    #[maybe_async]


The resulting token will be saved internally if the cache-token feature is enabled?

Yes. This function calls write_token_cache at the end so it will be written (only if cached tokens are enabled, as you may see in said function's definition)

ramsayleung · 2021-05-13T14:29:38Z

src/code_auth.rs

+        let token = self.fetch_access_token(&data).await?;
+        self.token = Some(token);
+
+        self.write_token_cache()


Same as above

ramsayleung · 2021-05-13T14:35:42Z

src/code_auth.rs

+///
+/// [reference]: https://developer.spotify.com/documentation/general/guides/authorization-guide/#authorization-code-flow
+/// [example-main]: https://github.com/ramsayleung/rspotify/blob/master/examples/code_auth.rs
+/// [example-webapp]: https://github.com/ramsayleung/rspotify/tree/master/examples/webapp


I think it will be better if we link reference with relative paths instead of HTTP links, which will be easily broken if I change username or repository

I don't think we can link to the examples with relative paths, as they aren't uploaded to docs.rs (AFAIK). Or maybe I've misunderstood you?

ramsayleung · 2021-05-13T14:36:11Z

src/code_auth.rs

+}
+
+/// This client has access to the base methods.
+#[maybe_async(?Send)]


Why would we need the Send trait?

This will probably explain it better than I can: https://docs.rs/async-trait/0.1.50/async_trait/#non-threadsafe-futures. I basically had to add + Send to each generic type in the trait functions, so passing (?Send) to the macro does that automatically.

ramsayleung · 2021-05-13T14:40:19Z

src/endpoints/oauth.rs

+    ///
+    /// Note: this method requires the `cli` feature.
+    #[cfg(feature = "cli")]
+    fn get_code_from_user(&self, url: &str) -> ClientResult<String> {


Not sure it's suitable to put get_code_from_user into OAuthClient trait, since we could get code without token and cred parameters. Same as parse_response_code.

But get_code_from_user doesn't really make sense if you're not using oauth, right? You need the code for the authorization flows, which is not needed in a base client.

ramsayleung · 2021-05-13T14:49:01Z

src/endpoints/pagination/iter.rs


 /// Alias for `Iterator<Item = T>`, since sync mode is enabled.
-pub trait Paginator<T>: Iterator<Item = T> {}
-impl<T, I: Iterator<Item = T>> Paginator<T> for I {}
+pub type Paginator<'a, T> = Box<dyn Iterator<Item = T> + 'a>;


I don't totally get it now why would we need to declare type pub type Paginator<'a, T> = Box<dyn Iterator<Item = T> + 'a>; instead of the previous one ?

pub trait Paginator<T>: Iterator<Item = T> {} impl<T, I: Iterator<Item = T>> Paginator<T> for I {}

Because we need some kinds of dynamic dispatch?

A trait-based implementation of these pagination methods is only possible if we use -> Box<dyn Paginator> instead of -> impl Paginator. The compiler will complain about it the same way we can't have -> impl Future<...> in traits, which we fix with async_trait (a macro that basically adds the Box automatically) . This is technically temporary and will be fixed in future Rust versions, but I'd give it at least a year until that happens, really. So we are basically forced to use Box + dyn if we want it to work for now.

This is sub-optimal because there's a slight runtime overhead in comparison to generics, but on the other hand there should be less code bloat due to generics (not that either of these really matter, IMO).

mistake

ramsayleung · 2021-05-13T15:02:19Z

Sorry for converting it from Draft to PR, my bad :(

What a huge PR, it takes me hours to give a rough review. The whole architecture of Auth process looks much clear and understandable than before, the current architecture looks good to me. Perhaps you could add some diagrams about the current Auth process.

Should ClientCredentialsSpotify be renamed to ClientCredsSpotify to shorten the name? It's quite lengthy and I did shorten CodeAuthorizationSpotify to CodeAuthSpotify.

Should CodeAuthSpotify be renamed to AuthCodeSpotify? It'd be more consistent with the real name, Authorization Code Flow

Should the endpoints module be renamed to clients? It's more fitting, as its contents aren't limited to endpoints anymore.

A big yes for all this suggestions.

By the ways, would you like splitting this PR into a Auth Rewrite PR and a Separate Crate PR, this PR has too many changes that it's a little hard to review line by line :)

marioortizmanero · 2021-05-14T00:08:14Z

What a huge PR, it takes me hours to give a rough review. The whole architecture of Auth process looks much clear and understandable than before, the current architecture looks good to me. Perhaps you could add some diagrams about the current Auth process.

Yes, it's a complete rewrite so the diff is just very hard to follow. I did want to add some diagrams but I wanted to make sure you were ok with it first of all. Here's a quick summary: we now have the implementations in traits: rspotify::clients::BaseClient and rspotify::clients::OAuthClient. We can then create custom structs which implement their own methods in order to follow whatever auth flow they're for, like ClientCredsSpotify.

This has some advantages and some inconvenients:

GOOD: Initializing a struct for a specific flow is easier and more natural in comparison (no builder pattern needed, as you can see in the examples). It's basically a higher level representation of the spotify clients.
GOOD: The implementation is somewhat cleaner, since we don't have to check if the struct has an OAuth struct in order to access certain methods. We know we can access the OAuth member in OAuthClient because of the get_oauth required implementation for the trait. This eliminates the need for an InvalidAuth error in the client.
GOOD: The library is more type-safe, making it harder to make a mistake by calling an OAuth method in an unauthorized client.
BAD: code reuse isn't that great. For example we can't override a method in BaseClient when implemeting its "child" OAuthClient, which would have been neat for read_token_cache, for example. This is fixable by using different names or by not implementing it in the trait and instead doing so in the client struct itself, but it's more of a workaround.

I've also had a bit of trouble sharing behavior between OAuth clients, namely AuthCodeSpotify and AuthCodePkceSpotify. I didn't know if e.g. prompt_for_token should go in the trait or in the client itself, and this happened for quite a few methods. I've ended up adding the methods to the trait if possible in order to reuse more code, but perhaps they should go in a different trait, as they might not fit as great as I thought. I don't know, it's subject to change.

This whole architecture would probably be nicer to implement if we had delegation in Rust. But that's not supported officially as of now (there are plans for it, though). We could use a delegation macro but I didn't find the problems I mentioned earlier important enough. Not sure what you think about that.
BAD: users now have to import rspotify::prelude::* always. Which is not a big deal, but we have to make sure the documentation is clear, now that the endpoints are separated into different traits and clients (and so is the documentation).

If you have any doubts do let me know. I know it's hard to follow the first time after a rewrite.

By the ways, would you like splitting this PR into a Auth Rewrite PR and a Separate Crate PR, this PR has too many changes that it's a little hard to review line by line :)

How important is this? It would take me a bit of work to undo some changes and make two PRs instead. Which is not a problem, but as I said, I'd need a few more days until I'm done (and I'll be busier soon due to finals, so it might take more). Just letting you know, I understand that this PR is huge.

ramsayleung · 2021-05-14T02:56:24Z

I didn't know if e.g. prompt_for_token should go in the trait or in the client itself, and this happened for quite a few methods. I've ended up adding the methods to the trait if possible in order to reuse more code, but perhaps they should go in a different trait, as they might not fit as great as I thought. I don't know, it's subject to change.

IMHO, moving methods like prompt_for_token, parse_response_code, get_code_from_user into a different trait would be a better idea, since they have weak relationship with Auth flow.

How important is this? It would take me a bit of work to undo some changes and make two PRs instead.

It doesn't really matter, it's low priority.

marioortizmanero · 2021-05-14T10:16:03Z

IMHO, moving methods like prompt_for_token, parse_response_code, get_code_from_user into a different trait would be a better idea, since they have weak relationship with Auth flow.

How so? You only have to prompt for the user's authorization with the auth flows. Or am I missing something? I don't see why you'd use any of these in the client credentials flow.

ramsayleung · 2021-05-14T15:09:22Z

How so? You only have to prompt for the user's authorization with the auth flows. Or am I missing something? I don't see why you'd use any of these in the client credentials flow.

Oooh, I misunderstand something, you are right.

marioortizmanero · 2021-06-19T10:36:08Z

Hello!

I've finally finished my finals, so I now have time to finish this. I've recreated the PR by splitting it up into four smaller PRs. The third one is still huge, but I don't think I can do anything about it; it separates the client into multiple ones, so the diff gets messed up. As I mentioned there, the endpoints themselves aren't changed, so you can ignore them.

If this still isn't digestible enough please let me know, I don't want to kill you with my reviews, @ramsayleung haha. I'll close this PR for now. I suggest you to start with the first PR up until the fourth. Afterwards we can merge all of them and push into master. But absolutely no rush.

Thanks a lot for the patience!

ramsayleung · 2021-06-21T02:00:51Z

If this still isn't digestible enough please let me know, I don't want to kill you with my reviews, @ramsayleung haha.

The good news is that there is a lot of reviews here, but it doesn't kill me. The bad news is it almost scares me out, haha :)

marioortizmanero added 3 commits April 29, 2021 00:01

moving http to a new crate

05f906b

rspotify-http now working more or less

68d625f

moving endpoints to respective traits

bbbb3af

marioortizmanero changed the base branch from master to separate-crates April 29, 2021 19:12

marioortizmanero marked this pull request as draft April 29, 2021 19:13

marioortizmanero mentioned this pull request Apr 29, 2021

Restructure the authentication process #173

Closed

marioortizmanero added 3 commits April 29, 2021 23:58

hit a wall with impl

1a4487b

it compiles... but at what cost

61edf5d

format

82f1825

marioortizmanero added 3 commits April 30, 2021 01:52

numerous fixes

b47f273

some fixes

1107624

Merge with master

3a0bb68

marioortizmanero added 6 commits April 30, 2021 15:12

format

02c4c0c

now compiling!

8b40904

fixed album example

7ece549

some fixes to the examples

ab39089

remove old oauth file

2c63e4c

fix second example

6d152a6

marioortizmanero added 9 commits April 30, 2021 23:24

tidy up examples, fix current_playing

01e581f

remove println

a825ae7

fixes most examples

4ac19cc

fix pagination

3ce657e

all examples work now

9112d75

fix tests

3b41775

fully fix pagination

17781e5

some more fixes

bf3235a

fix clippy

12a97ef

fix last CI issue

e6ed136

marioortizmanero mentioned this pull request May 9, 2021

Use Github Action to automatically publish crate to crate.io #211

Merged

4 tasks

ramsayleung previously approved these changes May 13, 2021

View reviewed changes

ramsayleung marked this pull request as ready for review May 13, 2021 14:51

ramsayleung self-requested a review May 13, 2021 14:52

ramsayleung marked this pull request as draft May 13, 2021 14:52

marioortizmanero added 6 commits May 14, 2021 01:42

add myself as an author!

b45b0f3

rename: code_auth -> auth_code

0c3faed

rename: CodeAuthSpotify -> AuthCodeSpotify

7523da4

rename: CodeAuthPkceSpotify -> AuthCodePkceSpotify

f44d9e8

rename: ClientCredentialsSpotify -> ClientCredsSpotify

1367931

rename: endpoints -> clients

8c016bc

marioortizmanero force-pushed the auth-rewrite branch from 261cd9d to 8c016bc Compare May 13, 2021 23:52

marioortizmanero added 3 commits May 14, 2021 02:26

fix maybe-async/is_sync in cargo.toml

5c1a7be

fix renames

56d5c52

format

bc0e0e4

marioortizmanero closed this Jun 19, 2021

marioortizmanero deleted the auth-rewrite branch June 19, 2021 10:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Auth restructure #207

[WIP] Auth restructure #207

marioortizmanero commented Apr 29, 2021 •

edited

marioortizmanero commented Apr 29, 2021 •

edited

marioortizmanero commented Apr 30, 2021

marioortizmanero commented Apr 30, 2021 •

edited

ramsayleung May 13, 2021

marioortizmanero May 13, 2021

ramsayleung May 13, 2021

marioortizmanero May 13, 2021 •

edited

ramsayleung May 13, 2021

marioortizmanero May 13, 2021

ramsayleung May 13, 2021

ramsayleung May 13, 2021

marioortizmanero May 13, 2021

ramsayleung May 13, 2021

marioortizmanero May 13, 2021 •

edited

ramsayleung May 13, 2021

marioortizmanero May 14, 2021

ramsayleung May 13, 2021

marioortizmanero May 13, 2021 •

edited

ramsayleung commented May 13, 2021

marioortizmanero commented May 14, 2021 •

edited

ramsayleung commented May 14, 2021 •

edited

marioortizmanero commented May 14, 2021

ramsayleung commented May 14, 2021

marioortizmanero commented Jun 19, 2021 •

edited

ramsayleung commented Jun 21, 2021


		self.token = Some(self.fetch_access_token(&data).await?);

		self.write_token_cache()

[WIP] Auth restructure #207

[WIP] Auth restructure #207

Conversation

marioortizmanero commented Apr 29, 2021 • edited

marioortizmanero commented Apr 29, 2021 • edited

marioortizmanero commented Apr 30, 2021

marioortizmanero commented Apr 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marioortizmanero May 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marioortizmanero May 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marioortizmanero May 13, 2021 • edited

Choose a reason for hiding this comment

ramsayleung commented May 13, 2021

marioortizmanero commented May 14, 2021 • edited

ramsayleung commented May 14, 2021 • edited

marioortizmanero commented May 14, 2021

ramsayleung commented May 14, 2021

marioortizmanero commented Jun 19, 2021 • edited

ramsayleung commented Jun 21, 2021

marioortizmanero commented Apr 29, 2021 •

edited

marioortizmanero commented Apr 29, 2021 •

edited

marioortizmanero commented Apr 30, 2021 •

edited

marioortizmanero May 13, 2021 •

edited

marioortizmanero May 13, 2021 •

edited

marioortizmanero May 13, 2021 •

edited

marioortizmanero commented May 14, 2021 •

edited

ramsayleung commented May 14, 2021 •

edited

marioortizmanero commented Jun 19, 2021 •

edited