diff --git a/models/fixtures/oauth2_application.yml b/models/fixtures/oauth2_application.yml index 2f38cb58b6..beae9137ad 100644 --- a/models/fixtures/oauth2_application.yml +++ b/models/fixtures/oauth2_application.yml @@ -14,7 +14,7 @@ name: "Test native app" client_id: "ce5a1322-42a7-11ed-b878-0242ac120002" client_secret: "$2a$10$UYRgUSgekzBp6hYe8pAdc.cgB4Gn06QRKsORUnIYTYQADs.YR/uvi" # bcrypt of "4MK8Na6R55smdCY0WuCCumZ6hjRPnGY5saWVRHHjJiA= - redirect_uris: '["http://127.0.0.1"]' + redirect_uris: '["b", "http://127.0.0.1"]' created_unix: 1546869730 updated_unix: 1546869730 confidential_client: false diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index b2d2abcf26..79751450b5 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -470,8 +470,9 @@ func AuthorizeOAuth(ctx *context.Context) { return } - // Redirect if user already granted access - if grant != nil { + // Redirect if user already granted access and the application is confidential. + // I.e. always require authorization for public clients as recommended by RFC 6749 Section 10.2 + if app.ConfidentialClient && grant != nil { code, err := grant.GenerateNewAuthorizationCode(ctx, form.RedirectURI, form.CodeChallenge, form.CodeChallengeMethod) if err != nil { handleServerError(ctx, form.State, form.RedirectURI) @@ -555,15 +556,30 @@ func GrantApplicationOAuth(ctx *context.Context) { ctx.ServerError("GetOAuth2ApplicationByClientID", err) return } - grant, err := app.CreateGrant(ctx, ctx.Doer.ID, form.Scope) + grant, err := app.GetGrantByUserID(ctx, ctx.Doer.ID) if err != nil { + handleServerError(ctx, form.State, form.RedirectURI) + return + } + if grant == nil { + grant, err = app.CreateGrant(ctx, ctx.Doer.ID, form.Scope) + if err != nil { + handleAuthorizeError(ctx, AuthorizeError{ + State: form.State, + ErrorDescription: "cannot create grant for user", + ErrorCode: ErrorCodeServerError, + }, form.RedirectURI) + return + } + } else if grant.Scope != form.Scope { handleAuthorizeError(ctx, AuthorizeError{ State: form.State, - ErrorDescription: "cannot create grant for user", + ErrorDescription: "a grant exists with different scope", ErrorCode: ErrorCodeServerError, }, form.RedirectURI) return } + if len(form.Nonce) > 0 { err := grant.SetNonce(ctx, form.Nonce) if err != nil { diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go index cffb8b5814..6d2ff8b1d3 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -79,6 +79,65 @@ func TestAuthorizeShow(t *testing.T) { htmlDoc.GetCSRF() } +func TestOAuth_AuthorizeConfidentialTwice(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // da7da3ba-9a13-4167-856f-3899de0b0138 a confidential client in models/fixtures/oauth2_application.yml + + // request authorization for the first time shows the grant page ... + authorizeURL := "/login/oauth/authorize?client_id=da7da3ba-9a13-4167-856f-3899de0b0138&redirect_uri=a&response_type=code&state=thestate" + req := NewRequest(t, "GET", authorizeURL) + ctx := loginUser(t, "user4") + resp := ctx.MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + htmlDoc.AssertElement(t, "#authorize-app", true) + + // ... and the user grants the authorization + req = NewRequestWithValues(t, "POST", "/login/oauth/grant", map[string]string{ + "_csrf": htmlDoc.GetCSRF(), + "client_id": "da7da3ba-9a13-4167-856f-3899de0b0138", + "redirect_uri": "a", + "state": "thestate", + "granted": "true", + }) + resp = ctx.MakeRequest(t, req, http.StatusSeeOther) + assert.Contains(t, test.RedirectURL(resp), "code=") + + // request authorization the second time and the grant page is not shown again, redirection happens immediately + req = NewRequest(t, "GET", authorizeURL) + resp = ctx.MakeRequest(t, req, http.StatusSeeOther) + assert.Contains(t, test.RedirectURL(resp), "code=") +} + +func TestOAuth_AuthorizePublicTwice(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // ce5a1322-42a7-11ed-b878-0242ac120002 is a public client in models/fixtures/oauth2_application.yml + authorizeURL := "/login/oauth/authorize?client_id=ce5a1322-42a7-11ed-b878-0242ac120002&redirect_uri=b&response_type=code&code_challenge_method=plain&code_challenge=CODE&state=thestate" + ctx := loginUser(t, "user4") + // a public client must be authorized every time + for _, name := range []string{"First", "Second"} { + t.Run(name, func(t *testing.T) { + req := NewRequest(t, "GET", authorizeURL) + resp := ctx.MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + htmlDoc.AssertElement(t, "#authorize-app", true) + + req = NewRequestWithValues(t, "POST", "/login/oauth/grant", map[string]string{ + "_csrf": htmlDoc.GetCSRF(), + "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", + "redirect_uri": "b", + "state": "thestate", + "granted": "true", + }) + resp = ctx.MakeRequest(t, req, http.StatusSeeOther) + assert.Contains(t, test.RedirectURL(resp), "code=") + }) + } +} + func TestAuthorizeRedirectWithExistingGrant(t *testing.T) { defer tests.PrepareTestEnv(t)() 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") @@ -480,7 +539,7 @@ func TestSignInOAuthCallbackRedirectToEscaping(t *testing.T) { gitlab := addAuthSource(t, authSourcePayloadGitLabCustom(gitlabName)) // - // Create a user as if it had been previously been created by the GitLab + // Create a user as if it had been previously created by the GitLab // authentication source. // userGitLabUserID := "5678"