Prefer HTTP basic authentication in OAuth2 client #9127
Conversation
jhass
changed the title
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
src/oauth2/client.cr
Outdated
| private getter host, client_id, client_secret, port, scheme, authorize_uri, | ||
| redirect_uri, auth_scheme, token_uri | ||
|
|
There was a problem hiding this comment.
What's the point of this private getter instead of using instance variables?
Smells like YAGNI :)
There was a problem hiding this comment.
I always prefer to use getters/setters instead of accessing instance variables directly.
There was a problem hiding this comment.
Well, refactoring like this might be considered out of scope of this PR. Secondly it's more costly, so if there's no good reason I'd advise against that.
There was a problem hiding this comment.
My reason for preferring getters/setters over instance variables even privately is that if the feature needs to change from being a variable to a method then you restrict your changes to the getter/setter.
Context: I once "won" an argument over this on a project where my view was that we should access private instance variables directly, and then lived to regret it. Since then, I've always had a horror of accessing instance variables directly (outside of constructors/getters/setters).
There was a problem hiding this comment.
OTOH if you have a horror of private accessors I can change them back. Your call.
There was a problem hiding this comment.
There was a problem hiding this comment.
@jhass I'd argue that in this case it's simply out of scope of this PR, but thanks for sharing this anyway ❤️
There was a problem hiding this comment.
@jhass Good to know
There was a problem hiding this comment.
@Sija private accessors dropped for using instance variables directly. I won't tell Bertrand Meyer if you don't.
…s per Sija's request.
| token = client.get_access_token_using_refresh_token(scope: "read_posts", refresh_token: "some_refresh_token") | ||
| token.extra.not_nil!["body"].should eq %("grant_type=refresh_token&refresh_token=some_refresh_token&scope=read_posts") | ||
| token.access_token.should eq "access_token" | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Is a newline missing after this line?
As discussed in forum post Can't get OAuth2 access token with grant type client credentials, the preferred behaviour should be to use HTTP Basic authentication to pass client credentials.
This PR provides the option to specify HTTP Basic or passing the credentials in the request body when creating the client, with HTTP Basic as the default.