Merge pull request #1709 from aiciobanu/basic-login-failed-message

Show an alert message when basic auth credentials are invalid

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

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)
 		}
 	}
 }

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 {

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
 	}
 

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 := "<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)))
 					},

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 @@
         <button class="button is-primary">Sign in</button>
       </form>
       {{ end }}
+
+      {{ if eq .StatusCode 400 401 }}
+      <div class="alert">
+        <span class="closebtn" onclick="this.parentElement.style.display='none';">&times;</span>
+        {{ if eq .StatusCode 400 }}
+        {{.StatusCode}}: Username cannot be empty
+        {{ else }}
+        {{.StatusCode}}: Invalid Username or Password
+        {{ end }}
+      </div> 
+      {{ end }}
+
     </div>
   </section>
 

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,

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())