From cbda3cf618330aafb6ab0652513feb678781e85a Mon Sep 17 00:00:00 2001 From: Alexandru Ciobanu Date: Thu, 30 Jun 2022 18:10:02 +0300 Subject: [PATCH 1/3] implement an error alert message for invalid basic auth credentials --- oauthproxy.go | 16 +++++++------- pkg/app/pagewriter/pagewriter.go | 8 +++---- pkg/app/pagewriter/sign_in.html | 34 ++++++++++++++++++++++++++++++ pkg/app/pagewriter/sign_in_page.go | 4 +++- 4 files changed, 49 insertions(+), 13 deletions(-) diff --git a/oauthproxy.go b/oauthproxy.go index cd2a3311..fd136271 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -568,26 +568,26 @@ func (p *OAuthProxy) SignInPage(rw http.ResponseWriter, req *http.Request, code redirectURL = "/" } - p.pageWriter.WriteSignInPage(rw, req, redirectURL) + p.pageWriter.WriteSignInPage(rw, req, redirectURL, code) } // ManualSignIn handles basic auth logins to the proxy -func (p *OAuthProxy) ManualSignIn(req *http.Request) (string, bool) { +func (p *OAuthProxy) ManualSignIn(req *http.Request) (string, bool, int) { if req.Method != "POST" || p.basicAuthValidator == nil { - return "", false + return "", false, http.StatusOK } user := req.FormValue("username") passwd := req.FormValue("password") if user == "" { - return "", false + return "", false, http.StatusBadRequest } // check auth if p.basicAuthValidator.Validate(user, passwd) { logger.PrintAuthf(user, req, logger.AuthSuccess, "Authenticated via HtpasswdFile") - return user, true + return user, true, http.StatusOK } logger.PrintAuthf(user, req, logger.AuthFailure, "Invalid authentication via HtpasswdFile") - return "", false + return "", false, http.StatusUnauthorized } // SignIn serves a page prompting users to sign in @@ -599,7 +599,7 @@ func (p *OAuthProxy) SignIn(rw http.ResponseWriter, req *http.Request) { return } - user, ok := p.ManualSignIn(req) + user, ok, statusCode := p.ManualSignIn(req) if ok { session := &sessionsapi.SessionState{User: user, Groups: p.basicAuthGroups} err = p.SaveSession(rw, req, session) @@ -614,7 +614,7 @@ func (p *OAuthProxy) SignIn(rw http.ResponseWriter, req *http.Request) { p.OAuthStart(rw, req) } else { // TODO - should we pass on /oauth2/sign_in query params to /oauth2/start? - p.SignInPage(rw, req, http.StatusOK) + p.SignInPage(rw, req, statusCode) } } } diff --git a/pkg/app/pagewriter/pagewriter.go b/pkg/app/pagewriter/pagewriter.go index 9bf7c2e2..8da82a87 100644 --- a/pkg/app/pagewriter/pagewriter.go +++ b/pkg/app/pagewriter/pagewriter.go @@ -10,7 +10,7 @@ import ( // It can also be used to write errors for the http.ReverseProxy used in the // upstream package. type Writer interface { - WriteSignInPage(rw http.ResponseWriter, req *http.Request, redirectURL string) + WriteSignInPage(rw http.ResponseWriter, req *http.Request, redirectURL string, statusCode int) WriteErrorPage(rw http.ResponseWriter, opts ErrorPageOpts) ProxyErrorHandler(rw http.ResponseWriter, req *http.Request, proxyErr error) WriteRobotsTxt(rw http.ResponseWriter, req *http.Request) @@ -108,7 +108,7 @@ func NewWriter(opts Opts) (Writer, error) { // If any of the funcs are not provided, a default implementation will be used. // This is primarily for us in testing. type WriterFuncs struct { - SignInPageFunc func(rw http.ResponseWriter, req *http.Request, redirectURL string) + SignInPageFunc func(rw http.ResponseWriter, req *http.Request, redirectURL string, statusCode int) ErrorPageFunc func(rw http.ResponseWriter, opts ErrorPageOpts) ProxyErrorFunc func(rw http.ResponseWriter, req *http.Request, proxyErr error) RobotsTxtfunc func(rw http.ResponseWriter, req *http.Request) @@ -117,9 +117,9 @@ type WriterFuncs struct { // WriteSignInPage implements the Writer interface. // If the SignInPageFunc is provided, this will be used, else a default // implementation will be used. -func (w *WriterFuncs) WriteSignInPage(rw http.ResponseWriter, req *http.Request, redirectURL string) { +func (w *WriterFuncs) WriteSignInPage(rw http.ResponseWriter, req *http.Request, redirectURL string, statusCode int) { if w.SignInPageFunc != nil { - w.SignInPageFunc(rw, req, redirectURL) + w.SignInPageFunc(rw, req, redirectURL, statusCode) return } diff --git a/pkg/app/pagewriter/sign_in.html b/pkg/app/pagewriter/sign_in.html index b68966c5..5aafada1 100644 --- a/pkg/app/pagewriter/sign_in.html +++ b/pkg/app/pagewriter/sign_in.html @@ -18,6 +18,28 @@ .logo-box { margin: 1.5rem 3rem; } + .alert { + padding: 5px; + background-color: #f44336; /* Red */ + color: white; + margin-bottom: 5px; + border-radius: 5px + } + /* The close button */ + .closebtn { + margin-left: 10px; + color: white; + font-weight: bold; + float: right; + font-size: 22px; + line-height: 20px; + cursor: pointer; + transition: 0.3s; + } + /* When moving the mouse over the close button */ + .closebtn:hover { + color: black; + } footer a { text-decoration: underline; } @@ -62,6 +84,18 @@ {{ end }} + + {{ if eq .StatusCode 400 401 }} +
+ × + {{ if eq .StatusCode 400 }} + {{.StatusCode}}: Username cannot be empty + {{ else }} + {{.StatusCode}}: Invalid Username or Password + {{ end }} +
+ {{ end }} + diff --git a/pkg/app/pagewriter/sign_in_page.go b/pkg/app/pagewriter/sign_in_page.go index 87870f89..ad2cdf15 100644 --- a/pkg/app/pagewriter/sign_in_page.go +++ b/pkg/app/pagewriter/sign_in_page.go @@ -54,12 +54,13 @@ type signInPageWriter struct { // WriteSignInPage writes the sign-in page to the given response writer. // It uses the redirectURL to be able to set the final destination for the user post login. -func (s *signInPageWriter) WriteSignInPage(rw http.ResponseWriter, req *http.Request, redirectURL string) { +func (s *signInPageWriter) WriteSignInPage(rw http.ResponseWriter, req *http.Request, redirectURL string, statusCode int) { // We allow unescaped template.HTML since it is user configured options /* #nosec G203 */ t := struct { ProviderName string SignInMessage template.HTML + StatusCode int CustomLogin bool Redirect string Version string @@ -69,6 +70,7 @@ func (s *signInPageWriter) WriteSignInPage(rw http.ResponseWriter, req *http.Req }{ ProviderName: s.providerName, SignInMessage: template.HTML(s.signInMessage), + StatusCode: statusCode, CustomLogin: s.displayLoginForm, Redirect: redirectURL, Version: s.version, From 52cf16284355b63ab9688ae06a11e93beeee4159 Mon Sep 17 00:00:00 2001 From: Alexandru Ciobanu Date: Thu, 30 Jun 2022 18:11:43 +0300 Subject: [PATCH 2/3] added tests for basic auth alert message feature --- oauthproxy_test.go | 53 +++++++++++++++++++++++++ pkg/app/pagewriter/pagewriter_test.go | 8 ++-- pkg/app/pagewriter/sign_in_page_test.go | 4 +- 3 files changed, 59 insertions(+), 6 deletions(-) diff --git a/oauthproxy_test.go b/oauthproxy_test.go index 218b4426..b2abe379 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -657,6 +657,59 @@ func TestManualSignInStoresUserGroupsInTheSession(t *testing.T) { assert.Equal(t, userGroups, s.Groups) } +type ManualSignInValidator struct{} + +func (ManualSignInValidator) Validate(user, password string) bool { + switch { + case user == "admin" && password == "adminPass": + return true + default: + return false + } +} + +func ManualSignInWithCredentials(t *testing.T, user, pass string) int { + opts := baseTestOptions() + err := validation.Validate(opts) + if err != nil { + t.Fatal(err) + } + + proxy, err := NewOAuthProxy(opts, func(email string) bool { + return true + }) + if err != nil { + t.Fatal(err) + } + + proxy.basicAuthValidator = ManualSignInValidator{} + + rw := httptest.NewRecorder() + formData := url.Values{} + formData.Set("username", user) + formData.Set("password", pass) + signInReq, _ := http.NewRequest(http.MethodPost, "/oauth2/sign_in", strings.NewReader(formData.Encode())) + signInReq.Header.Add("Content-Type", "application/x-www-form-urlencoded") + proxy.ServeHTTP(rw, signInReq) + + return rw.Code +} + +func TestManualSignInEmptyUsernameAlert(t *testing.T) { + statusCode := ManualSignInWithCredentials(t, "", "") + assert.Equal(t, http.StatusBadRequest, statusCode) +} + +func TestManualSignInInvalidCredentialsAlert(t *testing.T) { + statusCode := ManualSignInWithCredentials(t, "admin", "") + assert.Equal(t, http.StatusUnauthorized, statusCode) +} + +func TestManualSignInCorrectCredentials(t *testing.T) { + statusCode := ManualSignInWithCredentials(t, "admin", "adminPass") + assert.Equal(t, http.StatusFound, statusCode) +} + func TestSignInPageIncludesTargetRedirect(t *testing.T) { sipTest, err := NewSignInPageTest(false) if err != nil { diff --git a/pkg/app/pagewriter/pagewriter_test.go b/pkg/app/pagewriter/pagewriter_test.go index 2adedd19..cfea153b 100644 --- a/pkg/app/pagewriter/pagewriter_test.go +++ b/pkg/app/pagewriter/pagewriter_test.go @@ -57,7 +57,7 @@ var _ = Describe("Writer", func() { It("Writes the default sign in template", func() { recorder := httptest.NewRecorder() - writer.WriteSignInPage(recorder, request, "/redirect") + writer.WriteSignInPage(recorder, request, "/redirect", http.StatusOK) body, err := ioutil.ReadAll(recorder.Result().Body) Expect(err).ToNot(HaveOccurred()) @@ -104,7 +104,7 @@ var _ = Describe("Writer", func() { It("Writes the custom sign in template", func() { recorder := httptest.NewRecorder() - writer.WriteSignInPage(recorder, request, "/redirect") + writer.WriteSignInPage(recorder, request, "/redirect", http.StatusOK) body, err := ioutil.ReadAll(recorder.Result().Body) Expect(err).ToNot(HaveOccurred()) @@ -151,7 +151,7 @@ var _ = Describe("Writer", func() { rw := httptest.NewRecorder() req := httptest.NewRequest("", "/sign-in", nil) redirectURL := "" - in.writer.WriteSignInPage(rw, req, redirectURL) + in.writer.WriteSignInPage(rw, req, redirectURL, http.StatusOK) Expect(rw.Result().StatusCode).To(Equal(in.expectedStatus)) @@ -166,7 +166,7 @@ var _ = Describe("Writer", func() { }), Entry("With an override function", writerFuncsTableInput{ writer: &WriterFuncs{ - SignInPageFunc: func(rw http.ResponseWriter, req *http.Request, redirectURL string) { + SignInPageFunc: func(rw http.ResponseWriter, req *http.Request, redirectURL string, statusCode int) { rw.WriteHeader(202) rw.Write([]byte(fmt.Sprintf("%s %s", req.URL.Path, redirectURL))) }, diff --git a/pkg/app/pagewriter/sign_in_page_test.go b/pkg/app/pagewriter/sign_in_page_test.go index 804f45b0..05c0b2e9 100644 --- a/pkg/app/pagewriter/sign_in_page_test.go +++ b/pkg/app/pagewriter/sign_in_page_test.go @@ -54,7 +54,7 @@ var _ = Describe("SignIn Page", func() { Context("WriteSignInPage", func() { It("Writes the template to the response writer", func() { recorder := httptest.NewRecorder() - signInPage.WriteSignInPage(recorder, request, "/redirect") + signInPage.WriteSignInPage(recorder, request, "/redirect", http.StatusOK) body, err := ioutil.ReadAll(recorder.Result().Body) Expect(err).ToNot(HaveOccurred()) @@ -68,7 +68,7 @@ var _ = Describe("SignIn Page", func() { signInPage.template = tmpl recorder := httptest.NewRecorder() - signInPage.WriteSignInPage(recorder, request, "/redirect") + signInPage.WriteSignInPage(recorder, request, "/redirect", http.StatusOK) body, err := ioutil.ReadAll(recorder.Result().Body) Expect(err).ToNot(HaveOccurred()) From 33a3a602bc6ea8c55b5562ea5bd06da1af6d4e65 Mon Sep 17 00:00:00 2001 From: Alexandru Ciobanu Date: Wed, 13 Jul 2022 16:26:30 +0300 Subject: [PATCH 3/3] added `CHANGELOG.md` entry for #1709 --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d5a6379a..ca7e088a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ ## Changes since v7.3.0 +- [#1709](https://github.com/oauth2-proxy/oauth2-proxy/pull/1709) Show an alert message when basic auth credentials are invalid (@aiciobanu) + # V7.3.0 ## Release Highlights