v2/auth: add Basic authentication and refactor #159
Conversation
steveej
force-pushed
the
pr/auth-refactor-and-add-basic
branch
from
a385d51
to
d7ab100
Compare
src/v2/auth.rs
Outdated
| .ok_or_else(|| Error::from(format!("method not found in {}", header)))? | ||
| .as_str(); | ||
|
|
||
| let serialized_content = serde_json::Value::Object(serde_json::Map::from_iter( |
There was a problem hiding this comment.
This is somewhat inconvenient. I'm thinking of assembling a JSON string using plain string formatting and skipping the Value::Object route.
There was a problem hiding this comment.
Added 68da0dc on top which implements the plain string route. I like the aspect that we don't need the intermediate Value and I can't spot any weakness in the approach. I'd appreciate thoughts here.
There was a problem hiding this comment.
@lucab any opinion on this one?
There was a problem hiding this comment.
[sorry I didn't see this comment in review mode in GH]
The main problem I see with the logic in the fixup commit is when either key or value contains some characters that may need to be escaped in JSON-encoding (e.g. "). serde serialization takes care of that for you and is in general more robust.
But I do agree this code is a bit unwieldy. I think the source of the pain is the round-trip through JSON and serde. As a manual alternative, I think you could shove the key-value pairs into a map, drain the well-known values you expect in order to build the object, and warn if there are any leftover keys in the map.
There was a problem hiding this comment.
I found a compromise by wrapping the strings in a serde_json::Value::String. See this playground. WDYT?
There was a problem hiding this comment.
I pushed this suggestion into db88916 and am proposing it as the final solution. It doesn't have the escaping bug you detected with the previous revision, and it doesn't do the round trip. PTAL and let me know if I should squash 😉
There was a problem hiding this comment.
Yeah, it looks better than the initial version. Feel free to squash in.
steveej
force-pushed
the
pr/auth-refactor-and-add-basic
branch
from
d7ab100
to
e2d187c
Compare
steveej
force-pushed
the
pr/auth-refactor-and-add-basic
branch
from
68da0dc
to
f73cb89
Compare
steveej
force-pushed
the
pr/auth-refactor-and-add-basic
branch
from
f73cb89
to
42e0416
Compare
|
Thanks for the review @lucab! I addressed all comments in a fixup commit. I'll think about #159 (comment) a bit more. |
steveej
force-pushed
the
pr/auth-refactor-and-add-basic
branch
4 times, most recently
from
486bc63
to
a4e54b7
Compare
steveej
force-pushed
the
pr/auth-refactor-and-add-basic
branch
from
a4e54b7
to
db88916
Compare
To add some context to this, @LalatenduMohanty and I went over the code and the changes together using screen-sharing for about two hours 😉 |
|
Yes, it also looks good to me. Feel free to squash the fixup and self-merge. |
This adds Basic authentication support. Along the way, the auth module has been refactored with new enums and structs to model the supported authentication mechanisms and make the response header parsing more robust. ***Breaking API changes*** * The `Client::login` and `Client::authenticate` methods have been merged into the latter. * `Client::is_auth` now performs a pure authentication test without attempting to authenticate, thus it no longer takes a token.
steveej
force-pushed
the
pr/auth-refactor-and-add-basic
branch
from
db88916
to
16fc318
Compare
This adds Basic authentication support.
Along the way, the auth module has been refactored with new enums and
structs to model the supported authentication mechanisms and make the
response header parsing more robust.
Breaking API changes
Client::loginandClient::authenticatemethods have beenmerged into the latter.
Client::is_authnow performs a pure authentication test withoutattempting to authenticate, thus it no longer takes a token.