From b9f992ee118f4d55e3460816d904b2eb924e19fa Mon Sep 17 00:00:00 2001
From: Astrid Jahn <astrid.jahn@gesundheitscloud.de>
Date: Wed, 20 Dec 2017 10:44:38 +0100
Subject: [PATCH] Handle ErrNotFound separately in GetClient result

The GetClient method should return a not found error if no matching
client was found. However, callers of this method expect nonexistent
clients to be returned as nil without an error. This mismatch means that
an authorize request with an incorrect client ID will get a response
stating there was an unexpected server error. Separate error handling of
ErrNotFound is needed.
---
 access.go         |  4 ++++
 access_test.go    | 29 +++++++++++++++++++++++++++++
 authorize.go      |  4 ++++
 authorize_test.go | 39 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 76 insertions(+)

diff --git a/access.go b/access.go
index ce5f9df..0add2c1 100644
--- a/access.go
+++ b/access.go
@@ -535,6 +535,10 @@ func (s *Server) FinishAccessRequest(w *Response, r *http.Request, ar *AccessReq
 // storage. Sets an error on the response if auth fails or a server error occurs.
 func getClient(auth *BasicAuth, storage Storage, w *Response) Client {
 	client, err := storage.GetClient(auth.Username)
+	if err == ErrNotFound {
+		w.SetError(E_UNAUTHORIZED_CLIENT, "")
+		return nil
+	}
 	if err != nil {
 		w.SetError(E_SERVER_ERROR, "")
 		w.InternalError = err
diff --git a/access_test.go b/access_test.go
index def71e8..34bca19 100644
--- a/access_test.go
+++ b/access_test.go
@@ -303,6 +303,35 @@ func TestGetClientWithoutMatcher(t *testing.T) {
 		if client != nil {
 			t.Errorf("Expected error, got client: %v", client)
 		}
+
+		if !w.IsError {
+			t.Error("No error in response")
+		}
+
+		if w.ErrorId != E_UNAUTHORIZED_CLIENT {
+			t.Errorf("Expected error %v, got %v", E_UNAUTHORIZED_CLIENT, w.ErrorId)
+		}
+	}
+
+	// Ensure nonexistent client fails
+	{
+		auth := &BasicAuth{
+			Username: "nonexistent",
+			Password: "nonexistent",
+		}
+		w := &Response{}
+		client := getClient(auth, storage, w)
+		if client != nil {
+			t.Errorf("Expected error, got client: %v", client)
+		}
+
+		if !w.IsError {
+			t.Error("No error in response")
+		}
+
+		if w.ErrorId != E_UNAUTHORIZED_CLIENT {
+			t.Errorf("Expected error %v, got %v", E_UNAUTHORIZED_CLIENT, w.ErrorId)
+		}
 	}
 
 	// Ensure good secret works
diff --git a/authorize.go b/authorize.go
index fe319ab..f5c67d1 100644
--- a/authorize.go
+++ b/authorize.go
@@ -124,6 +124,10 @@ func (s *Server) HandleAuthorizeRequest(w *Response, r *http.Request) *Authorize
 
 	// must have a valid client
 	ret.Client, err = w.Storage.GetClient(r.Form.Get("client_id"))
+	if err == ErrNotFound {
+		w.SetErrorState(E_UNAUTHORIZED_CLIENT, "", ret.State)
+		return nil
+	}
 	if err != nil {
 		w.SetErrorState(E_SERVER_ERROR, "", ret.State)
 		w.InternalError = err
diff --git a/authorize_test.go b/authorize_test.go
index d8e0ad2..c4165f8 100644
--- a/authorize_test.go
+++ b/authorize_test.go
@@ -88,6 +88,45 @@ func TestAuthorizeToken(t *testing.T) {
 	}
 }
 
+func TestAuthorizeTokenWithInvalidClient(t *testing.T) {
+	sconfig := NewServerConfig()
+	sconfig.AllowedAuthorizeTypes = AllowedAuthorizeType{TOKEN}
+	server := NewServer(sconfig, NewTestingStorage())
+	server.AuthorizeTokenGen = &TestingAuthorizeTokenGen{}
+	server.AccessTokenGen = &TestingAccessTokenGen{}
+	resp := server.NewResponse()
+	redirectUri := "http://redirecturi.com"
+
+	req, err := http.NewRequest("GET", "http://localhost:14000/appauth", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	req.Form = make(url.Values)
+	req.Form.Set("response_type", string(TOKEN))
+	req.Form.Set("client_id", "invalidclient")
+	req.Form.Set("state", "a")
+	req.Form.Set("redirect_uri", redirectUri)
+
+	if ar := server.HandleAuthorizeRequest(resp, req); ar != nil {
+		ar.Authorized = true
+		server.FinishAuthorizeRequest(resp, req, ar)
+	}
+
+	if !resp.IsError {
+		t.Fatalf("Response should be an error")
+	}
+
+	if resp.ErrorId != E_UNAUTHORIZED_CLIENT {
+		t.Fatalf("Incorrect error in response: %v", resp.ErrorId)
+	}
+
+	usedRedirectUrl, redirectErr := resp.GetRedirectUrl()
+
+	if redirectErr == nil && usedRedirectUrl == redirectUri {
+		t.Fatalf("Response must not redirect to the provided redirect URL for an invalid client")
+	}
+}
+
 func TestAuthorizeCodePKCERequired(t *testing.T) {
 	sconfig := NewServerConfig()
 	sconfig.RequirePKCEForPublicClients = true