Merge pull request '[v1.21/forgejo] RFC 6749 Section 10.2 conformance' (#4047) from earl-warren/forgejo:wip-v1.21-oauth into v1.21/forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/4047
Reviewed-by: oliverpool <oliverpool@noreply.codeberg.org>

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

routers/web/auth/oauth.go

@@ -468,8 +468,9 @@ func AuthorizeOAuth(ctx *context.Context) {
 		return
 	}
 
-	// Redirect if user already granted access
+	// Redirect if user already granted access and the application is confidential.
-	if grant != nil {
+	// 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)
@@ -543,7 +544,13 @@ 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,
@@ -552,6 +559,15 @@ func GrantApplicationOAuth(ctx *context.Context) {
 			}, form.RedirectURI)
 			return
 		}
+	} else if grant.Scope != form.Scope {
+		handleAuthorizeError(ctx, AuthorizeError{
+			State:            form.State,
+			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 {

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"