OAuth process made by a struct, allowing customization options. #1462
Conversation
There was a problem hiding this comment.
Nice job! Overall pretty clean code, had just some nitpicks and minor comments here and there.
Thanks for adding full documentation out of the box. To put the cherry on top you could add #![warn(missing_docs)] at the top of the crate to enforce documentation of all public items in the future.
I suppose this will supersede #1396?
There was a problem hiding this comment.
When I created this I envisioned it being useful as a standalone library function, therefore I intentionally did not make it async. I personally find it frustrating when stuff is async for no reason and it means the caller now needs to drag something like tokio in too.
What about a crate feature then, that would gives the option to op-out (or in) from async? I personally like that methods are exposed as async, as the user then can decide how to handle it. But I can also see the appeal of having a sync blocking option. |
Or maybe, thanks to the freedom provided by the Struct, there could be 4 methods (two sync and two async) coexisting and each user could choose the one that suits its needs. |
|
...or: the async function just wraps a blocking spawn and waits for it to return. My train of thought to select the best alternative:
|
|
I've added methods to handle this both async and sync, completed the documentation for the module and some other minor changes. |
There was a problem hiding this comment.
Thanks a lot for documenting the whole crate :D
even tho I just suggested it as addition not as requirement
Some general point on your documentation. If you look at other rust docs, it usually starts with a short summary and then follows up with some details or remarks in paragraphs. It's most noticeably at the methods/function. Most other docs work quite well tho^^
@roderickvd Just took a looked into the topic and it seems |
@photovoltex Don't worry! I did it with pleasure. |
Co-authored-by: Felix Prillwitz <[email protected]>
Co-authored-by: Felix Prillwitz <[email protected]>
Co-authored-by: Felix Prillwitz <[email protected]>
Co-authored-by: Felix Prillwitz <[email protected]>
Co-authored-by: Felix Prillwitz <[email protected]>
Co-authored-by: Nick Steel <[email protected]>
Co-authored-by: Nick Steel <[email protected]>
| } | ||
|
|
||
| /// Synchronously obtain a new valid OAuth token from `refresh_token` | ||
| pub fn refresh_token(&self, refresh_token: &str) -> Result<OAuthToken, OAuthError> { |
There was a problem hiding this comment.
The name refresh_token as function and parameter might be a bit misleading, as it is refers to the token that refreshes the access_token, but here it also the method which refreshes the access_token and not referring to the refresh_token itself. Probably something like get_refreshed_access_token might be a better explanation without overloading two meaning, but it's totally not wrong how it is, so we could also keep it that way.
| } | ||
| }; | ||
| last_credentials = Some(Credentials::with_access_token(access_token)); | ||
| ) |
There was a problem hiding this comment.
| ) | |
| ) | |
| .open_in_browser() |
|
Finally got around to taking it for a spin. I'm not sure I like what |
You can configure which fields it redacts: #[derive(Redact)]
pub enum Credentials {
Login {
email: String,
// only redact the password
#[redact]
password: String,
}
}You can also set how it redacts. |
|
I thought that was the behavior requested in previous comments. To not log sensible information but with the library having all the information to do authorized requests. If its expected to get all the info through the standard output, I don't think |
|
Ok, that's good. Ideally it would redact just the tokens (not the other things) and only for logs. Ideally there would still be a way to print out the full un-redacted info, which is what the examples should do. |
|
I was initially thinking about something like this when I notate the idea^^;. println!("Test: {:*<8.4}", "some long string that will only print out `some` with 4 following `*`");
ref:
When I saw the veil change I thought it might just make sense or work, but I somewhat didn't expect it to censor the debug output and well didn't tested it myself. In general it probably makes more sense to keep the info visible and rather keep a look out what we print onto stdout with something similar to the above formatting. But this solution is only viable for a single print/log, so users of the lib would need to handle it on their own if they want to keep the token redacted form the logs. |
|
We could undo this aspect and sort it out in a later PR? |
Hello!
I'm currently working on a side project using this library. While working on it, I felt the need to open the auth URL automatically and customize the message returned by the socket. I added this functionality in my own fork, but I thought you might find it useful, so here's a Pull Request for it. If you don’t think it’s useful or it doesn’t meet your standards, feel free to close it!
I implemented it using a builder pattern that returns an OAuthClient with two functions:
get_access_token()andrefresh_token().The good old
get_access_token()is now marked for deprecation to allow for a smooth transition to the new structs. I also removed a weird overhead in the function, where it spawned a thread only to wait for its completion right after—so I figured it could just be removed.I’ve also modified the OAuth example so you can see how this implementation works.