Add authentication support #31
Conversation
The error message in this case appears to have changed not to include the DNS resolver address, resulting in the expected error failure. To get around that and make the test more robust, check just for the "no such host" fragment inside the error message, rather than attempting to match the entire message. Signed-off-by: Sergei Trofimov <[email protected]>
setrofim
force-pushed
the
auth
branch
2 times, most recently
from
625b28e to
e0dea50
Compare
| } | ||
|
|
||
| // Type returns the string representing the type name (used by pflag). | ||
| func (o *Method) Type() string { |
There was a problem hiding this comment.
This is not clear to me the purpose and scope of this method
There was a problem hiding this comment.
it's a method required by the pflag.Value interface, which the auth.Method type implements
There was a problem hiding this comment.
We should clarify a bit more in the comment to make it better!
There was a problem hiding this comment.
The comment already states that it's used by pflag
|
|
||
| o.ClientID = decoded.ClientID | ||
| o.ClientSecret = decoded.ClientSecret | ||
| o.TokenURL = decoded.TokenURL |
There was a problem hiding this comment.
Should we not check whether the URL is a valid URL before setting it?
There was a problem hiding this comment.
maybe mapstructure:"token_url", valid:"url" ?
There was a problem hiding this comment.
I'm not familiar with the valid tag, and by google-foo seems to be failing me. Could you provide a link to an explanation of how that works?
There was a problem hiding this comment.
Validation at the moment happends inside validate() which is called just below. While it may techincally be more go-ish ("gogonic"?) to not until valudation, it would make the code less neat (we'd ether have to mix validation code with rest making the method less readable, or expose the currently interannly-defied temporary struct so that we can attach the validate method to it), and I don't see a concrete benefit?
auth/oauth2.go
Outdated
| } | ||
|
|
||
| if len(decoded.Rest) > 0 { | ||
| return errors.New("unexpected fields in config") |
There was a problem hiding this comment.
Why should we hard fail here if the Rest bytes are supplied.
We should just Warn and continue if the required essential parameters are valid and satisfy the constraints which we set.
There was a problem hiding this comment.
This is library code, not application code -- we should let the calling code handle it (it may have ideosyncratic mechansims for logging/warning).
Please note that the input here does not come directly from the end-user, it is a map generated by the calling code, so it's on that code to make sure the map is correct. It is up to the calling code to decide whether to allow additional fields in the input (and just ignore them when populating the map, or remove them if the map is generated from end-user input), or to report the error and fail. We should not be making that decision in the library code.
| o.ClientSecret = decoded.ClientSecret | ||
| o.TokenURL = decoded.TokenURL | ||
| o.Username = decoded.Username | ||
| o.Password = decoded.Password |
There was a problem hiding this comment.
Is there a requirement on Password encoding, like minimum characters and specific characters?
There was a problem hiding this comment.
I guess any such policy should be set on the service side rather than here
There was a problem hiding this comment.
Correct -- this would be up to the particular OAuth2 service, we should not enforce anything here.
| package auth | ||
|
|
||
| type IAuthenticator interface { | ||
| Configure(cfg map[string]interface{}) error |
There was a problem hiding this comment.
I think, we should have a Validate Method on the IAuthenticator Interface
There was a problem hiding this comment.
To what end? Validation is performed as part of Configure() which is the only way for the user to alter the state of an IAuthenticator
There was a problem hiding this comment.
True that this is part of Configure that you call Validator.
I am ok with this!
Add support for authentication with the remote service. Basic HTTP and OAuth2 schemes are supported (as well as "passthrough" which disables authentication). This is implemented by associating an IAuthorizer with the Client, which sets the Authrization HTTP header in Client's requests. Signed-off-by: Sergei Trofimov <[email protected]>
|
Comments addressed. |
Add support for authentication with the remote service. Basic HTTP and
OAuth2 schemes are supported (as well as "passthrough" which disables
authentication).
This is implemented by associating an IAuthorizer interface with the Client, which
sets the Authorization HTTP header in Client's requests.
(Note: while not directly depending on it, this only makes sense in the context of veraison/services#186)