From e84558b0931309cf1f4f2767bc47296483b9b3e1 Mon Sep 17 00:00:00 2001 From: M Hickford Date: Wed, 12 Oct 2022 05:22:43 +0100 Subject: [PATCH] Improve OAuth integration tests (#21390) In particular, test explicit error responses. No change to behaviour. Co-authored-by: wxiaoguang --- models/fixtures/oauth2_application.yml | 2 +- tests/integration/oauth_test.go | 164 ++++++++++++++++++++----- 2 files changed, 133 insertions(+), 33 deletions(-) diff --git a/models/fixtures/oauth2_application.yml b/models/fixtures/oauth2_application.yml index a13e20b10..34d5a8877 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 7fa26c814..9621acbbc 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) }