New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a POST request handler to the direct provider #57
Conversation
beb9c52
to
80355b0
Compare
Pull Request Test Coverage Report for Build 144177369
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, thx.
A few minor suggestions. And in addition, the readme section about direct auth should be updated accordingly
provider/direct.go
Outdated
if contentType == "application/json" { | ||
var credentials Credentials | ||
if err := json.NewDecoder(r.Body).Decode(&credentials); err != nil { | ||
return Credentials{}, fmt.Errorf("failed to parse request body: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg/errors already in the list of deps and used in other places. i.e. errors.Wrap(err, "failed to parse request body") will be consistent
provider/direct.go
Outdated
@@ -13,6 +16,13 @@ import ( | |||
"github.com/go-pkgz/auth/token" | |||
) | |||
|
|||
const ( | |||
// MaxHTTPBodySize defines max http body size | |||
MaxHTTPBodySize = megabyte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MaxHTTPBodySize = 1024*1024
will be easier to read
provider/direct.go
Outdated
@@ -37,21 +47,48 @@ func (f CredCheckerFunc) Check(user, password string) (ok bool, err error) { | |||
return f(user, password) | |||
} | |||
|
|||
// Credentials holds user credentials | |||
type Credentials struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this struct supposed to be exposed. If so, making it credentials
makes more sense
provider/direct.go
Outdated
func (p DirectHandler) LoginHandler(w http.ResponseWriter, r *http.Request) { | ||
user, password := r.URL.Query().Get("user"), r.URL.Query().Get("passwd") | ||
aud := r.URL.Query().Get("aud") | ||
if r.Body != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably, moving this body wrapping to getCredentials will make this part closer to the consumer and more readable
80355b0
to
52634b8
Compare
52634b8
to
6087a84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thx.
In some case it's required to send user provided input data in POST request. This PR adds
application/x-www-form-urlencoded
andapplication/json
encoded body processors to the direct handler.