Improve OAuth integration tests (#21390)

In particular, test explicit error responses.

No change to behaviour.

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

2 files changed, 133 insertions, 33 deletions

diff --git a/models/fixtures/oauth2_application.yml b/models/fixtures/oauth2_application.yml
index a13e20b10e..34d5a88777 100644
--- a/models/fixtures/oauth2_application.yml
+++ b/models/fixtures/oauth2_application.yml
@@ -4,6 +4,6 @@
   name: "Test"
   client_id: "da7da3ba-9a13-4167-856f-3899de0b0138"
   client_secret: "$2a$10$UYRgUSgekzBp6hYe8pAdc.cgB4Gn06QRKsORUnIYTYQADs.YR/uvi" # bcrypt of "4MK8Na6R55smdCY0WuCCumZ6hjRPnGY5saWVRHHjJiA=
-  redirect_uris: '["a"]'
+  redirect_uris: '["a", "https://example.com/xyzzy"]'
   created_unix: 1546869730
   updated_unix: 1546869730
diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go
index 7fa26c8147..9621acbbcc 100644
--- a/tests/integration/oauth_test.go
+++ b/tests/integration/oauth_test.go
@@ -12,29 +12,59 @@ import (
 
 	"code.gitea.io/gitea/modules/json"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/routers/web/auth"
 	"code.gitea.io/gitea/tests"
 
 	"github.com/stretchr/testify/assert"
 )
 
-const defaultAuthorize = "/login/oauth/authorize?client_id=da7da3ba-9a13-4167-856f-3899de0b0138&redirect_uri=a&response_type=code&state=thestate"
-
-func TestNoClientID(t *testing.T) {
+func TestAuthorizeNoClientID(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 	req := NewRequest(t, "GET", "/login/oauth/authorize")
 	ctx := loginUser(t, "user2")
-	ctx.MakeRequest(t, req, http.StatusBadRequest)
+	resp := ctx.MakeRequest(t, req, http.StatusBadRequest)
+	assert.Contains(t, resp.Body.String(), "Client ID not registered")
+}
+
+func TestAuthorizeUnregisteredRedirect(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+	req := NewRequest(t, "GET", "/login/oauth/authorize?client_id=da7da3ba-9a13-4167-856f-3899de0b0138&redirect_uri=UNREGISTERED&response_type=code&state=thestate")
+	ctx := loginUser(t, "user1")
+	resp := ctx.MakeRequest(t, req, http.StatusBadRequest)
+	assert.Contains(t, resp.Body.String(), "Unregistered Redirect URI")
+}
+
+func TestAuthorizeUnsupportedResponseType(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+	req := NewRequest(t, "GET", "/login/oauth/authorize?client_id=da7da3ba-9a13-4167-856f-3899de0b0138&redirect_uri=a&response_type=UNEXPECTED&state=thestate")
+	ctx := loginUser(t, "user1")
+	resp := ctx.MakeRequest(t, req, http.StatusSeeOther)
+	u, err := resp.Result().Location()
+	assert.NoError(t, err)
+	assert.Equal(t, "unsupported_response_type", u.Query().Get("error"))
+	assert.Equal(t, "Only code response type is supported.", u.Query().Get("error_description"))
+}
+
+func TestAuthorizeUnsupportedCodeChallengeMethod(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+	req := NewRequest(t, "GET", "/login/oauth/authorize?client_id=da7da3ba-9a13-4167-856f-3899de0b0138&redirect_uri=a&response_type=code&state=thestate&code_challenge_method=UNEXPECTED")
+	ctx := loginUser(t, "user1")
+	resp := ctx.MakeRequest(t, req, http.StatusSeeOther)
+	u, err := resp.Result().Location()
+	assert.NoError(t, err)
+	assert.Equal(t, "invalid_request", u.Query().Get("error"))
+	assert.Equal(t, "unsupported code challenge method", u.Query().Get("error_description"))
 }
 
-func TestLoginRedirect(t *testing.T) {
+func TestAuthorizeLoginRedirect(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 	req := NewRequest(t, "GET", "/login/oauth/authorize")
 	assert.Contains(t, MakeRequest(t, req, http.StatusSeeOther).Body.String(), "/user/login")
 }
 
-func TestShowAuthorize(t *testing.T) {
+func TestAuthorizeShow(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
-	req := NewRequest(t, "GET", defaultAuthorize)
+	req := NewRequest(t, "GET", "/login/oauth/authorize?client_id=da7da3ba-9a13-4167-856f-3899de0b0138&redirect_uri=a&response_type=code&state=thestate")
 	ctx := loginUser(t, "user4")
 	resp := ctx.MakeRequest(t, req, http.StatusOK)
 
@@ -43,15 +73,17 @@ func TestShowAuthorize(t *testing.T) {
 	htmlDoc.GetCSRF()
 }
 
-func TestRedirectWithExistingGrant(t *testing.T) {
+func TestAuthorizeRedirectWithExistingGrant(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
-	req := NewRequest(t, "GET", defaultAuthorize)
+	req := NewRequest(t, "GET", "/login/oauth/authorize?client_id=da7da3ba-9a13-4167-856f-3899de0b0138&redirect_uri=https%3A%2F%2Fexample.com%2Fxyzzy&response_type=code&state=thestate")
 	ctx := loginUser(t, "user1")
 	resp := ctx.MakeRequest(t, req, http.StatusSeeOther)
 	u, err := resp.Result().Location()
 	assert.NoError(t, err)
 	assert.Equal(t, "thestate", u.Query().Get("state"))
 	assert.Truef(t, len(u.Query().Get("code")) > 30, "authorization code '%s' should be longer then 30", u.Query().Get("code"))
+	u.RawQuery = ""
+	assert.Equal(t, "https://example.com/xyzzy", u.String())
 }
 
 func TestAccessTokenExchange(t *testing.T) {
@@ -62,7 +94,7 @@ func TestAccessTokenExchange(t *testing.T) {
 		"client_secret": "4MK8Na6R55smdCY0WuCCumZ6hjRPnGY5saWVRHHjJiA=",
 		"redirect_uri":  "a",
 		"code":          "authcode",
-		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", // test PKCE additionally
+		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt",
 	})
 	resp := MakeRequest(t, req, http.StatusOK)
 	type response struct {
@@ -78,7 +110,7 @@ func TestAccessTokenExchange(t *testing.T) {
 	assert.True(t, len(parsed.RefreshToken) > 10)
 }
 
-func TestAccessTokenExchangeWithoutPKCE(t *testing.T) {
+func TestAccessTokenExchangeJSON(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 	req := NewRequestWithJSON(t, "POST", "/login/oauth/access_token", map[string]string{
 		"grant_type":    "authorization_code",
@@ -86,7 +118,7 @@ func TestAccessTokenExchangeWithoutPKCE(t *testing.T) {
 		"client_secret": "4MK8Na6R55smdCY0WuCCumZ6hjRPnGY5saWVRHHjJiA=",
 		"redirect_uri":  "a",
 		"code":          "authcode",
-		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", // test PKCE additionally
+		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt",
 	})
 	resp := MakeRequest(t, req, http.StatusOK)
 	type response struct {
@@ -102,16 +134,20 @@ func TestAccessTokenExchangeWithoutPKCE(t *testing.T) {
 	assert.True(t, len(parsed.RefreshToken) > 10)
 }
 
-func TestAccessTokenExchangeJSON(t *testing.T) {
+func TestAccessTokenExchangeWithoutPKCE(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
-	req := NewRequestWithJSON(t, "POST", "/login/oauth/access_token", map[string]string{
+	req := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{
 		"grant_type":    "authorization_code",
 		"client_id":     "da7da3ba-9a13-4167-856f-3899de0b0138",
 		"client_secret": "4MK8Na6R55smdCY0WuCCumZ6hjRPnGY5saWVRHHjJiA=",
 		"redirect_uri":  "a",
 		"code":          "authcode",
 	})
-	MakeRequest(t, req, http.StatusBadRequest)
+	resp := MakeRequest(t, req, http.StatusBadRequest)
+	parsedError := new(auth.AccessTokenError)
+	assert.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError))
+	assert.Equal(t, "unauthorized_client", string(parsedError.ErrorCode))
+	assert.Equal(t, "failed PKCE code challenge", parsedError.ErrorDescription)
 }
 
 func TestAccessTokenExchangeWithInvalidCredentials(t *testing.T) {
@@ -123,9 +159,14 @@ func TestAccessTokenExchangeWithInvalidCredentials(t *testing.T) {
 		"client_secret": "4MK8Na6R55smdCY0WuCCumZ6hjRPnGY5saWVRHHjJiA=",
 		"redirect_uri":  "a",
 		"code":          "authcode",
-		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", // test PKCE additionally
+		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt",
 	})
-	MakeRequest(t, req, http.StatusBadRequest)
+	resp := MakeRequest(t, req, http.StatusBadRequest)
+	parsedError := new(auth.AccessTokenError)
+	assert.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError))
+	assert.Equal(t, "invalid_client", string(parsedError.ErrorCode))
+	assert.Equal(t, "cannot load client with client id: '???'", parsedError.ErrorDescription)
+
 	// invalid client secret
 	req = NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{
 		"grant_type":    "authorization_code",
@@ -133,9 +174,14 @@ func TestAccessTokenExchangeWithInvalidCredentials(t *testing.T) {
 		"client_secret": "???",
 		"redirect_uri":  "a",
 		"code":          "authcode",
-		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", // test PKCE additionally
+		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt",
 	})
-	MakeRequest(t, req, http.StatusBadRequest)
+	resp = MakeRequest(t, req, http.StatusBadRequest)
+	parsedError = new(auth.AccessTokenError)
+	assert.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError))
+	assert.Equal(t, "unauthorized_client", string(parsedError.ErrorCode))
+	assert.Equal(t, "invalid client secret", parsedError.ErrorDescription)
+
 	// invalid redirect uri
 	req = NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{
 		"grant_type":    "authorization_code",
@@ -143,9 +189,14 @@ func TestAccessTokenExchangeWithInvalidCredentials(t *testing.T) {
 		"client_secret": "4MK8Na6R55smdCY0WuCCumZ6hjRPnGY5saWVRHHjJiA=",
 		"redirect_uri":  "???",
 		"code":          "authcode",
-		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", // test PKCE additionally
+		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt",
 	})
-	MakeRequest(t, req, http.StatusBadRequest)
+	resp = MakeRequest(t, req, http.StatusBadRequest)
+	parsedError = new(auth.AccessTokenError)
+	assert.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError))
+	assert.Equal(t, "unauthorized_client", string(parsedError.ErrorCode))
+	assert.Equal(t, "unexpected redirect URI", parsedError.ErrorDescription)
+
 	// invalid authorization code
 	req = NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{
 		"grant_type":    "authorization_code",
@@ -153,9 +204,14 @@ func TestAccessTokenExchangeWithInvalidCredentials(t *testing.T) {
 		"client_secret": "4MK8Na6R55smdCY0WuCCumZ6hjRPnGY5saWVRHHjJiA=",
 		"redirect_uri":  "a",
 		"code":          "???",
-		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", // test PKCE additionally
+		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt",
 	})
-	MakeRequest(t, req, http.StatusBadRequest)
+	resp = MakeRequest(t, req, http.StatusBadRequest)
+	parsedError = new(auth.AccessTokenError)
+	assert.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError))
+	assert.Equal(t, "unauthorized_client", string(parsedError.ErrorCode))
+	assert.Equal(t, "client is not authorized", parsedError.ErrorDescription)
+
 	// invalid grant_type
 	req = NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{
 		"grant_type":    "???",
@@ -163,9 +219,13 @@ func TestAccessTokenExchangeWithInvalidCredentials(t *testing.T) {
 		"client_secret": "4MK8Na6R55smdCY0WuCCumZ6hjRPnGY5saWVRHHjJiA=",
 		"redirect_uri":  "a",
 		"code":          "authcode",
-		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", // test PKCE additionally
+		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt",
 	})
-	MakeRequest(t, req, http.StatusBadRequest)
+	resp = MakeRequest(t, req, http.StatusBadRequest)
+	parsedError = new(auth.AccessTokenError)
+	assert.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError))
+	assert.Equal(t, "unsupported_grant_type", string(parsedError.ErrorCode))
+	assert.Equal(t, "Only refresh_token or authorization_code grant type is supported", parsedError.ErrorDescription)
 }
 
 func TestAccessTokenExchangeWithBasicAuth(t *testing.T) {
@@ -174,7 +234,7 @@ func TestAccessTokenExchangeWithBasicAuth(t *testing.T) {
 		"grant_type":    "authorization_code",
 		"redirect_uri":  "a",
 		"code":          "authcode",
-		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", // test PKCE additionally
+		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt",
 	})
 	req.Header.Add("Authorization", "Basic ZGE3ZGEzYmEtOWExMy00MTY3LTg1NmYtMzg5OWRlMGIwMTM4OjRNSzhOYTZSNTVzbWRDWTBXdUNDdW1aNmhqUlBuR1k1c2FXVlJISGpKaUE9")
 	resp := MakeRequest(t, req, http.StatusOK)
@@ -195,19 +255,54 @@ func TestAccessTokenExchangeWithBasicAuth(t *testing.T) {
 		"grant_type":    "authorization_code",
 		"redirect_uri":  "a",
 		"code":          "authcode",
-		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", // test PKCE additionally
+		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt",
 	})
 	req.Header.Add("Authorization", "Basic ZGE3ZGEzYmEtOWExMy00MTY3LTg1NmYtMzg5OWRlMGIwMTM4OmJsYWJsYQ==")
-	MakeRequest(t, req, http.StatusBadRequest)
+	resp = MakeRequest(t, req, http.StatusBadRequest)
+	parsedError := new(auth.AccessTokenError)
+	assert.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError))
+	assert.Equal(t, "unauthorized_client", string(parsedError.ErrorCode))
+	assert.Equal(t, "invalid client secret", parsedError.ErrorDescription)
 
 	// missing header
 	req = NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{
 		"grant_type":    "authorization_code",
 		"redirect_uri":  "a",
 		"code":          "authcode",
-		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", // test PKCE additionally
+		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt",
+	})
+	resp = MakeRequest(t, req, http.StatusBadRequest)
+	parsedError = new(auth.AccessTokenError)
+	assert.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError))
+	assert.Equal(t, "invalid_client", string(parsedError.ErrorCode))
+	assert.Equal(t, "cannot load client with client id: ''", parsedError.ErrorDescription)
+
+	// client_id inconsistent with Authorization header
+	req = NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{
+		"grant_type":   "authorization_code",
+		"redirect_uri": "a",
+		"code":         "authcode",
+		"client_id":    "inconsistent",
+	})
+	req.Header.Add("Authorization", "Basic ZGE3ZGEzYmEtOWExMy00MTY3LTg1NmYtMzg5OWRlMGIwMTM4OjRNSzhOYTZSNTVzbWRDWTBXdUNDdW1aNmhqUlBuR1k1c2FXVlJISGpKaUE9")
+	resp = MakeRequest(t, req, http.StatusBadRequest)
+	parsedError = new(auth.AccessTokenError)
+	assert.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError))
+	assert.Equal(t, "invalid_request", string(parsedError.ErrorCode))
+	assert.Equal(t, "client_id in request body inconsistent with Authorization header", parsedError.ErrorDescription)
+
+	// client_secret inconsistent with Authorization header
+	req = NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{
+		"grant_type":    "authorization_code",
+		"redirect_uri":  "a",
+		"code":          "authcode",
+		"client_secret": "inconsistent",
 	})
-	MakeRequest(t, req, http.StatusBadRequest)
+	req.Header.Add("Authorization", "Basic ZGE3ZGEzYmEtOWExMy00MTY3LTg1NmYtMzg5OWRlMGIwMTM4OjRNSzhOYTZSNTVzbWRDWTBXdUNDdW1aNmhqUlBuR1k1c2FXVlJISGpKaUE9")
+	parsedError = new(auth.AccessTokenError)
+	assert.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError))
+	assert.Equal(t, "invalid_request", string(parsedError.ErrorCode))
+	assert.Equal(t, "client_id in request body inconsistent with Authorization header", parsedError.ErrorDescription)
 }
 
 func TestRefreshTokenInvalidation(t *testing.T) {
@@ -218,7 +313,7 @@ func TestRefreshTokenInvalidation(t *testing.T) {
 		"client_secret": "4MK8Na6R55smdCY0WuCCumZ6hjRPnGY5saWVRHHjJiA=",
 		"redirect_uri":  "a",
 		"code":          "authcode",
-		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", // test PKCE additionally
+		"code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt",
 	})
 	resp := MakeRequest(t, req, http.StatusOK)
 	type response struct {
@@ -256,6 +351,11 @@ func TestRefreshTokenInvalidation(t *testing.T) {
 	refreshReq.Body = io.NopCloser(bytes.NewReader(bs))
 	MakeRequest(t, refreshReq, http.StatusOK)
 
+	// repeat request should fail
 	refreshReq.Body = io.NopCloser(bytes.NewReader(bs))
-	MakeRequest(t, refreshReq, http.StatusBadRequest)
+	resp = MakeRequest(t, refreshReq, http.StatusBadRequest)
+	parsedError := new(auth.AccessTokenError)
+	assert.NoError(t, json.Unmarshal(resp.Body.Bytes(), parsedError))
+	assert.Equal(t, "unauthorized_client", string(parsedError.ErrorCode))
+	assert.Equal(t, "token was already used", parsedError.ErrorDescription)
 }