From 5efd05e5e21b3570b4447386aef909a366205fdd Mon Sep 17 00:00:00 2001 From: M Hickford Date: Sun, 2 Oct 2022 19:28:50 +0100 Subject: [PATCH 01/11] Support for OAuth client type public, ie. native apps --- .../content/doc/developers/oauth2-provider.md | 6 ++++ models/auth/oauth2.go | 28 ++++++++++++------- models/auth/oauth2_test.go | 1 + models/fixtures/oauth2_application.yml | 11 ++++++++ models/fixtures/oauth2_authorization_code.yml | 7 +++++ models/fixtures/oauth2_grant.yml | 10 ++++++- models/migrations/migrations.go | 2 ++ models/migrations/v226.go | 27 ++++++++++++++++++ modules/convert/convert.go | 1 + modules/structs/user_app.go | 2 ++ options/locale/locale_en-US.ini | 4 +-- routers/api/v1/user/app.go | 2 ++ routers/web/auth/oauth.go | 2 +- routers/web/user/setting/oauth2.go | 2 ++ services/forms/user_form.go | 5 ++-- templates/swagger/v1_json.tmpl | 8 ++++++ .../user/settings/applications_oauth2.tmpl | 4 +++ .../settings/applications_oauth2_edit.tmpl | 4 +++ tests/integration/api_oauth2_apps_test.go | 8 ++++++ tests/integration/oauth_test.go | 14 ++++++++++ 20 files changed, 131 insertions(+), 17 deletions(-) create mode 100644 models/migrations/v226.go diff --git a/docs/content/doc/developers/oauth2-provider.md b/docs/content/doc/developers/oauth2-provider.md index 9e6ab11742fb5..c6765f19e7aab 100644 --- a/docs/content/doc/developers/oauth2-provider.md +++ b/docs/content/doc/developers/oauth2-provider.md @@ -44,6 +44,12 @@ To use the Authorization Code Grant as a third party application it is required Currently Gitea does not support scopes (see [#4300](https://github.com/go-gitea/gitea/issues/4300)) and all third party applications will be granted access to all resources of the user and their organizations. +## Client types + +Gitea supports both confidential and public client types, [as defined by RFC 6749](https://datatracker.ietf.org/doc/html/rfc6749#section-2.1). + +For public clients, a redirect URI of a loopback IP address such as `http://127.0.0.1/` allows any port. Avoid using `localhost`, [as recommended by RFC 8252](https://datatracker.ietf.org/doc/html/rfc8252#section-8.3). + ## Example **Note:** This example does not use PKCE. diff --git a/models/auth/oauth2.go b/models/auth/oauth2.go index 73c250d4af8a8..8e608717034e8 100644 --- a/models/auth/oauth2.go +++ b/models/auth/oauth2.go @@ -31,6 +31,8 @@ type OAuth2Application struct { Name string ClientID string `xorm:"unique"` ClientSecret string + // https://datatracker.ietf.org/doc/html/rfc6749#section-2.1 + Confidential bool `xorm:"NOT NULL DEFAULT TRUE"` RedirectURIs []string `xorm:"redirect_uris JSON TEXT"` CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` @@ -57,15 +59,17 @@ func (app *OAuth2Application) PrimaryRedirectURI() string { // ContainsRedirectURI checks if redirectURI is allowed for app func (app *OAuth2Application) ContainsRedirectURI(redirectURI string) bool { - uri, err := url.Parse(redirectURI) - // ignore port for http loopback uris following https://datatracker.ietf.org/doc/html/rfc8252#section-7.3 - if err == nil && uri.Scheme == "http" && uri.Port() != "" { - ip := net.ParseIP(uri.Hostname()) - if ip != nil && ip.IsLoopback() { - // strip port - uri.Host = uri.Hostname() - if util.IsStringInSlice(uri.String(), app.RedirectURIs, true) { - return true + if !app.Confidential { + uri, err := url.Parse(redirectURI) + // ignore port for http loopback uris following https://datatracker.ietf.org/doc/html/rfc8252#section-7.3 + if err == nil && uri.Scheme == "http" && uri.Port() != "" { + ip := net.ParseIP(uri.Hostname()) + if ip != nil && ip.IsLoopback() { + // strip port + uri.Host = uri.Hostname() + if util.IsStringInSlice(uri.String(), app.RedirectURIs, true) { + return true + } } } } @@ -163,6 +167,7 @@ func GetOAuth2ApplicationsByUserID(ctx context.Context, userID int64) (apps []*O type CreateOAuth2ApplicationOptions struct { Name string UserID int64 + Confidential bool RedirectURIs []string } @@ -174,6 +179,7 @@ func CreateOAuth2Application(ctx context.Context, opts CreateOAuth2ApplicationOp Name: opts.Name, ClientID: clientID, RedirectURIs: opts.RedirectURIs, + Confidential: opts.Confidential, } if err := db.Insert(ctx, app); err != nil { return nil, err @@ -186,6 +192,7 @@ type UpdateOAuth2ApplicationOptions struct { ID int64 Name string UserID int64 + Confidential bool RedirectURIs []string } @@ -207,6 +214,7 @@ func UpdateOAuth2Application(opts UpdateOAuth2ApplicationOptions) (*OAuth2Applic app.Name = opts.Name app.RedirectURIs = opts.RedirectURIs + app.Confidential = opts.Confidential if err = updateOAuth2Application(ctx, app); err != nil { return nil, err @@ -217,7 +225,7 @@ func UpdateOAuth2Application(opts UpdateOAuth2ApplicationOptions) (*OAuth2Applic } func updateOAuth2Application(ctx context.Context, app *OAuth2Application) error { - if _, err := db.GetEngine(ctx).ID(app.ID).Update(app); err != nil { + if _, err := db.GetEngine(ctx).ID(app.ID).UseBool("Confidential").Update(app); err != nil { return err } return nil diff --git a/models/auth/oauth2_test.go b/models/auth/oauth2_test.go index 3815cb3b2c195..5bef2edd9f8f7 100644 --- a/models/auth/oauth2_test.go +++ b/models/auth/oauth2_test.go @@ -46,6 +46,7 @@ func TestOAuth2Application_ContainsRedirectURI(t *testing.T) { func TestOAuth2Application_ContainsRedirectURI_WithPort(t *testing.T) { app := &auth_model.OAuth2Application{ RedirectURIs: []string{"http://127.0.0.1/", "http://::1/", "http://192.168.0.1/", "http://intranet/", "https://127.0.0.1/"}, + Confidential: false, } // http loopback uris should ignore port diff --git a/models/fixtures/oauth2_application.yml b/models/fixtures/oauth2_application.yml index a13e20b10e2a3..bea7b1002e19d 100644 --- a/models/fixtures/oauth2_application.yml +++ b/models/fixtures/oauth2_application.yml @@ -7,3 +7,14 @@ redirect_uris: '["a"]' created_unix: 1546869730 updated_unix: 1546869730 + confidential: true +- + id: 2 + uid: 2 + 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"]' + created_unix: 1546869730 + updated_unix: 1546869730 + confidential: false diff --git a/models/fixtures/oauth2_authorization_code.yml b/models/fixtures/oauth2_authorization_code.yml index 2abce16354a1b..d29502164e67f 100644 --- a/models/fixtures/oauth2_authorization_code.yml +++ b/models/fixtures/oauth2_authorization_code.yml @@ -6,3 +6,10 @@ redirect_uri: "a" valid_until: 3546869730 +- id: 2 + grant_id: 4 + code: "authcodepublic" + code_challenge: "CjvyTLSdR47G5zYenDA-eDWW4lRrO8yvjcWwbD_deOg" # Code Verifier: N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt + code_challenge_method: "S256" + redirect_uri: "http://127.0.0.1/" + valid_until: 3546869730 diff --git a/models/fixtures/oauth2_grant.yml b/models/fixtures/oauth2_grant.yml index e52a2bce959e6..e63286878b20f 100644 --- a/models/fixtures/oauth2_grant.yml +++ b/models/fixtures/oauth2_grant.yml @@ -20,4 +20,12 @@ counter: 1 scope: "openid profile email" created_unix: 1546869730 - updated_unix: 1546869730 \ No newline at end of file + updated_unix: 1546869730 + +- id: 4 + user_id: 99 + application_id: 2 + counter: 1 + scope: "whatever" + created_unix: 1546869730 + updated_unix: 1546869730 diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 28ffc998860bf..ef9be849b887f 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -413,6 +413,8 @@ var migrations = []Migration{ NewMigration("Add badges to users", createUserBadgesTable), // v225 -> v226 NewMigration("Alter gpg_key/public_key content TEXT fields to MEDIUMTEXT", alterPublicGPGKeyContentFieldsToMediumText), + // v226 -> v227 + NewMigration("Add confidential column default true to OAuth2Application table", addConfidentialColumnToOAuth2ApplicationTable), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v226.go b/models/migrations/v226.go new file mode 100644 index 0000000000000..666c86cd1b32e --- /dev/null +++ b/models/migrations/v226.go @@ -0,0 +1,27 @@ +// Copyright 2022 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import ( + "code.gitea.io/gitea/modules/timeutil" + "xorm.io/xorm" +) + +// addConfidentialColumnToOAuth2ApplicationTable: add Confidential column, setting existing rows to true +func addConfidentialColumnToOAuth2ApplicationTable(x *xorm.Engine) error { + type OAuth2Application struct { + ID int64 `xorm:"pk autoincr"` + UID int64 `xorm:"INDEX"` + Name string + ClientID string `xorm:"unique"` + ClientSecret string + Confidential bool `xorm:"NOT NULL DEFAULT TRUE"` + RedirectURIs []string `xorm:"redirect_uris JSON TEXT"` + CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` + UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` + } + + return x.Sync(new(OAuth2Application)) +} diff --git a/modules/convert/convert.go b/modules/convert/convert.go index af759cb9388bf..ffe391dbbd489 100644 --- a/modules/convert/convert.go +++ b/modules/convert/convert.go @@ -396,6 +396,7 @@ func ToOAuth2Application(app *auth.OAuth2Application) *api.OAuth2Application { Name: app.Name, ClientID: app.ClientID, ClientSecret: app.ClientSecret, + Confidential: app.Confidential, RedirectURIs: app.RedirectURIs, Created: app.CreatedUnix.AsTime(), } diff --git a/modules/structs/user_app.go b/modules/structs/user_app.go index 44df5a6a49546..2614e5bb46082 100644 --- a/modules/structs/user_app.go +++ b/modules/structs/user_app.go @@ -31,6 +31,7 @@ type CreateAccessTokenOption struct { // CreateOAuth2ApplicationOptions holds options to create an oauth2 application type CreateOAuth2ApplicationOptions struct { Name string `json:"name" binding:"Required"` + Confidential bool `json:"confidential"` RedirectURIs []string `json:"redirect_uris" binding:"Required"` } @@ -41,6 +42,7 @@ type OAuth2Application struct { Name string `json:"name"` ClientID string `json:"client_id"` ClientSecret string `json:"client_secret"` + Confidential bool `json:"confidential"` RedirectURIs []string `json:"redirect_uris"` Created time.Time `json:"created"` } diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 1dba1d71d8ffe..08ee14ef0964b 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -735,9 +735,7 @@ create_oauth2_application_button = Create Application create_oauth2_application_success = You've successfully created a new OAuth2 application. update_oauth2_application_success = You've successfully updated the OAuth2 application. oauth2_application_name = Application Name -oauth2_select_type = Which application type fits? -oauth2_type_web = Web (e.g. Node.JS, Tomcat, Go) -oauth2_type_native = Native (e.g. Mobile, Desktop, Browser) +oauth2_confidential = Confidential. Select for apps that keep the secret confidential, such as web apps. Do not select for native apps including desktop and mobile apps. oauth2_redirect_uri = Redirect URI save_application = Save oauth2_client_id = Client ID diff --git a/routers/api/v1/user/app.go b/routers/api/v1/user/app.go index a94db79239344..fa16c2f22ab4b 100644 --- a/routers/api/v1/user/app.go +++ b/routers/api/v1/user/app.go @@ -216,6 +216,7 @@ func CreateOauth2Application(ctx *context.APIContext) { Name: data.Name, UserID: ctx.Doer.ID, RedirectURIs: data.RedirectURIs, + Confidential: data.Confidential, }) if err != nil { ctx.Error(http.StatusBadRequest, "", "error creating oauth2 application") @@ -367,6 +368,7 @@ func UpdateOauth2Application(ctx *context.APIContext) { UserID: ctx.Doer.ID, ID: appID, RedirectURIs: data.RedirectURIs, + Confidential: data.Confidential, }) if err != nil { if auth_model.IsErrOauthClientIDInvalid(err) || auth_model.IsErrOAuthApplicationNotFound(err) { diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index d145150535e6e..8d304a4fcf2d2 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -685,7 +685,7 @@ func handleAuthorizationCode(ctx *context.Context, form forms.AccessTokenForm, s }) return } - if !app.ValidateClientSecret([]byte(form.ClientSecret)) { + if app.Confidential && !app.ValidateClientSecret([]byte(form.ClientSecret)) { handleAccessTokenError(ctx, AccessTokenError{ ErrorCode: AccessTokenErrorCodeUnauthorizedClient, ErrorDescription: "invalid client secret", diff --git a/routers/web/user/setting/oauth2.go b/routers/web/user/setting/oauth2.go index db76a12f1874a..f72b3c5ba2ec0 100644 --- a/routers/web/user/setting/oauth2.go +++ b/routers/web/user/setting/oauth2.go @@ -38,6 +38,7 @@ func OAuthApplicationsPost(ctx *context.Context) { Name: form.Name, RedirectURIs: []string{form.RedirectURI}, UserID: ctx.Doer.ID, + Confidential: form.Confidential, }) if err != nil { ctx.ServerError("CreateOAuth2Application", err) @@ -72,6 +73,7 @@ func OAuthApplicationsEdit(ctx *context.Context) { Name: form.Name, RedirectURIs: []string{form.RedirectURI}, UserID: ctx.Doer.ID, + Confidential: form.Confidential, }); err != nil { ctx.ServerError("UpdateOAuth2Application", err) return diff --git a/services/forms/user_form.go b/services/forms/user_form.go index 8ce1d85c57781..9c9ebc09ce16b 100644 --- a/services/forms/user_form.go +++ b/services/forms/user_form.go @@ -379,8 +379,9 @@ func (f *NewAccessTokenForm) Validate(req *http.Request, errs binding.Errors) bi // EditOAuth2ApplicationForm form for editing oauth2 applications type EditOAuth2ApplicationForm struct { - Name string `binding:"Required;MaxSize(255)" form:"application_name"` - RedirectURI string `binding:"Required" form:"redirect_uri"` + Name string `binding:"Required;MaxSize(255)" form:"application_name"` + RedirectURI string `binding:"Required" form:"redirect_uri"` + Confidential bool `form:"confidential"` } // Validate validates the fields diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index b0e94b8bb5b33..84fabe73ec7df 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -14639,6 +14639,10 @@ "description": "CreateOAuth2ApplicationOptions holds options to create an oauth2 application", "type": "object", "properties": { + "confidential": { + "type": "boolean", + "x-go-name": "Confidential" + }, "name": { "type": "string", "x-go-name": "Name" @@ -17277,6 +17281,10 @@ "type": "string", "x-go-name": "ClientSecret" }, + "confidential": { + "type": "boolean", + "x-go-name": "Confidential" + }, "created": { "type": "string", "format": "date-time", diff --git a/templates/user/settings/applications_oauth2.tmpl b/templates/user/settings/applications_oauth2.tmpl index 5e53ed00ca13f..844975e8ca3cd 100644 --- a/templates/user/settings/applications_oauth2.tmpl +++ b/templates/user/settings/applications_oauth2.tmpl @@ -37,6 +37,10 @@ +
+ + +
diff --git a/templates/user/settings/applications_oauth2_edit.tmpl b/templates/user/settings/applications_oauth2_edit.tmpl index be3e78e46b8d2..51183be5f9000 100644 --- a/templates/user/settings/applications_oauth2_edit.tmpl +++ b/templates/user/settings/applications_oauth2_edit.tmpl @@ -42,6 +42,10 @@
+
+ + +
diff --git a/tests/integration/api_oauth2_apps_test.go b/tests/integration/api_oauth2_apps_test.go index fe3525724e26a..2336d130c6744 100644 --- a/tests/integration/api_oauth2_apps_test.go +++ b/tests/integration/api_oauth2_apps_test.go @@ -34,6 +34,7 @@ func testAPICreateOAuth2Application(t *testing.T) { RedirectURIs: []string{ "http://www.google.com", }, + Confidential: true, } req := NewRequestWithJSON(t, "POST", "/api/v1/user/applications/oauth2", &appBody) @@ -46,6 +47,7 @@ func testAPICreateOAuth2Application(t *testing.T) { assert.EqualValues(t, appBody.Name, createdApp.Name) assert.Len(t, createdApp.ClientSecret, 56) assert.Len(t, createdApp.ClientID, 36) + assert.True(t, createdApp.Confidential) assert.NotEmpty(t, createdApp.Created) assert.EqualValues(t, appBody.RedirectURIs[0], createdApp.RedirectURIs[0]) unittest.AssertExistsAndLoadBean(t, &auth.OAuth2Application{UID: user.ID, Name: createdApp.Name}) @@ -62,6 +64,7 @@ func testAPIListOAuth2Applications(t *testing.T) { RedirectURIs: []string{ "http://www.google.com", }, + Confidential: true, }) urlStr := fmt.Sprintf("/api/v1/user/applications/oauth2?token=%s", token) @@ -74,6 +77,7 @@ func testAPIListOAuth2Applications(t *testing.T) { assert.EqualValues(t, existApp.Name, expectedApp.Name) assert.EqualValues(t, existApp.ClientID, expectedApp.ClientID) + assert.Equal(t, existApp.Confidential, expectedApp.Confidential) assert.Len(t, expectedApp.ClientID, 36) assert.Empty(t, expectedApp.ClientSecret) assert.EqualValues(t, existApp.RedirectURIs[0], expectedApp.RedirectURIs[0]) @@ -112,6 +116,7 @@ func testAPIGetOAuth2Application(t *testing.T) { RedirectURIs: []string{ "http://www.google.com", }, + Confidential: true, }) urlStr := fmt.Sprintf("/api/v1/user/applications/oauth2/%d?token=%s", existApp.ID, token) @@ -124,6 +129,7 @@ func testAPIGetOAuth2Application(t *testing.T) { assert.EqualValues(t, existApp.Name, expectedApp.Name) assert.EqualValues(t, existApp.ClientID, expectedApp.ClientID) + assert.Equal(t, existApp.Confidential, expectedApp.Confidential) assert.Len(t, expectedApp.ClientID, 36) assert.Empty(t, expectedApp.ClientSecret) assert.Len(t, expectedApp.RedirectURIs, 1) @@ -148,6 +154,7 @@ func testAPIUpdateOAuth2Application(t *testing.T) { "http://www.google.com/", "http://www.github.com/", }, + Confidential: true, } urlStr := fmt.Sprintf("/api/v1/user/applications/oauth2/%d", existApp.ID) @@ -162,5 +169,6 @@ func testAPIUpdateOAuth2Application(t *testing.T) { assert.Len(t, expectedApp.RedirectURIs, 2) assert.EqualValues(t, expectedApp.RedirectURIs[0], appBody.RedirectURIs[0]) assert.EqualValues(t, expectedApp.RedirectURIs[1], appBody.RedirectURIs[1]) + assert.Equal(t, expectedApp.Confidential, appBody.Confidential) unittest.AssertExistsAndLoadBean(t, &auth.OAuth2Application{ID: expectedApp.ID, Name: expectedApp.Name}) } diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go index 7fa26c81474b5..ec4eca2eb5715 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -168,6 +168,20 @@ func TestAccessTokenExchangeWithInvalidCredentials(t *testing.T) { MakeRequest(t, req, http.StatusBadRequest) } +func TestAccessTokenExchangeForPublicClient(t *testing.T) { + defer tests.PrepareTestEnv(t)() + req := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ + "grant_type": "authorization_code", + "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", + // client_secret not required for public client + // redirect port may vary + "redirect_uri": "http://127.0.0.1:3456", + "code": "authcodepublic", + "code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", // test PKCE additionally + }) + MakeRequest(t, req, http.StatusOK) +} + func TestAccessTokenExchangeWithBasicAuth(t *testing.T) { defer tests.PrepareTestEnv(t)() req := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ From d7f3ccbd2ef5ddfd7157600da1ce2d8bd053bb46 Mon Sep 17 00:00:00 2001 From: M Hickford Date: Wed, 5 Oct 2022 18:01:57 +0100 Subject: [PATCH 02/11] Address review comments Require PKCE for public clients --- models/auth/oauth2.go | 3 +++ routers/web/auth/oauth.go | 17 +++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/models/auth/oauth2.go b/models/auth/oauth2.go index 8e608717034e8..a7642b336f357 100644 --- a/models/auth/oauth2.go +++ b/models/auth/oauth2.go @@ -31,7 +31,10 @@ type OAuth2Application struct { Name string ClientID string `xorm:"unique"` ClientSecret string + // OAuth defines both Confidential and Public client types // https://datatracker.ietf.org/doc/html/rfc6749#section-2.1 + // "Authorization servers MUST record the client type in the client registration details" + // https://datatracker.ietf.org/doc/html/rfc8252#section-8.4 Confidential bool `xorm:"NOT NULL DEFAULT TRUE"` RedirectURIs []string `xorm:"redirect_uris JSON TEXT"` CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index 8d304a4fcf2d2..a64c247634407 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -430,8 +430,21 @@ func AuthorizeOAuth(ctx *context.Context) { log.Error("Unable to save changes to the session: %v", err) } case "": - break + // "Authorization servers SHOULD reject authorization requests from native apps that don't use PKCE by returning an error message" + // https://datatracker.ietf.org/doc/html/rfc8252#section-8.1 + if !app.Confidential { + // "the authorization endpoint MUST return the authorization error response with the "error" value set to "invalid_request"" + // https://datatracker.ietf.org/doc/html/rfc7636#section-4.4.1 + handleAuthorizeError(ctx, AuthorizeError{ + ErrorCode: ErrorCodeInvalidRequest, + ErrorDescription: "", + State: form.State, + }, form.RedirectURI) + } + return default: + // "If the server supporting PKCE does not support the requested transformation, the authorization endpoint MUST return the authorization error response with "error" value set to "invalid_request"." + // https://www.rfc-editor.org/rfc/rfc7636#section-4.4.1 handleAuthorizeError(ctx, AuthorizeError{ ErrorCode: ErrorCodeInvalidRequest, ErrorDescription: "unsupported code challenge method", @@ -685,7 +698,7 @@ func handleAuthorizationCode(ctx *context.Context, form forms.AccessTokenForm, s }) return } - if app.Confidential && !app.ValidateClientSecret([]byte(form.ClientSecret)) { + if !app.ValidateClientSecret([]byte(form.ClientSecret)) { handleAccessTokenError(ctx, AccessTokenError{ ErrorCode: AccessTokenErrorCodeUnauthorizedClient, ErrorDescription: "invalid client secret", From eb87013e7fb74bbec9b1c4a94b71901be63affba Mon Sep 17 00:00:00 2001 From: M Hickford Date: Sat, 8 Oct 2022 04:31:36 +0100 Subject: [PATCH 03/11] Correct return --- routers/web/auth/oauth.go | 4 ++-- tests/integration/oauth_test.go | 19 +++++++++++++++---- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index ad1a426c91e38..a0caed985159b 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -437,11 +437,11 @@ func AuthorizeOAuth(ctx *context.Context) { // https://datatracker.ietf.org/doc/html/rfc7636#section-4.4.1 handleAuthorizeError(ctx, AuthorizeError{ ErrorCode: ErrorCodeInvalidRequest, - ErrorDescription: "", + ErrorDescription: "PKCE is required for public clients", State: form.State, }, form.RedirectURI) + return } - return default: // "If the server supporting PKCE does not support the requested transformation, the authorization endpoint MUST return the authorization error response with "error" value set to "invalid_request"." // https://www.rfc-editor.org/rfc/rfc7636#section-4.4.1 diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go index ec4eca2eb5715..7b55f9e0cb67e 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -171,15 +171,26 @@ func TestAccessTokenExchangeWithInvalidCredentials(t *testing.T) { func TestAccessTokenExchangeForPublicClient(t *testing.T) { defer tests.PrepareTestEnv(t)() req := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ - "grant_type": "authorization_code", - "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", - // client_secret not required for public client + "grant_type": "authorization_code", + "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", + "client_secret": "4MK8Na6R55smdCY0WuCCumZ6hjRPnGY5saWVRHHjJiA=", // redirect port may vary "redirect_uri": "http://127.0.0.1:3456", "code": "authcodepublic", - "code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", // test PKCE additionally + "code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", }) MakeRequest(t, req, http.StatusOK) + + req = NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ + "grant_type": "authorization_code", + "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", + "client_secret": "4MK8Na6R55smdCY0WuCCumZ6hjRPnGY5saWVRHHjJiA=", + // redirect port may vary + "redirect_uri": "http://127.0.0.1:3456", + "code": "authcodepublic", + // omit PKCE + }) + MakeRequest(t, req, http.StatusBadRequest) } func TestAccessTokenExchangeWithBasicAuth(t *testing.T) { From 5e9529d020e59cd951be6e374f37818c55e4da59 Mon Sep 17 00:00:00 2001 From: M Hickford Date: Mon, 10 Oct 2022 18:27:57 +0100 Subject: [PATCH 04/11] Improve unit test --- .../applications_oauth2_edit_form.tmpl | 2 +- .../settings/applications_oauth2_list.tmpl | 2 +- tests/integration/oauth_test.go | 36 ++++++------------- 3 files changed, 13 insertions(+), 27 deletions(-) diff --git a/templates/user/settings/applications_oauth2_edit_form.tmpl b/templates/user/settings/applications_oauth2_edit_form.tmpl index d28bd2d3563d7..207dee77f44fb 100644 --- a/templates/user/settings/applications_oauth2_edit_form.tmpl +++ b/templates/user/settings/applications_oauth2_edit_form.tmpl @@ -42,7 +42,7 @@
-
+
diff --git a/templates/user/settings/applications_oauth2_list.tmpl b/templates/user/settings/applications_oauth2_list.tmpl index 4631ff4a238b4..1fde1c7b7d669 100644 --- a/templates/user/settings/applications_oauth2_list.tmpl +++ b/templates/user/settings/applications_oauth2_list.tmpl @@ -33,7 +33,7 @@
-
+
diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go index 7b55f9e0cb67e..c679b98ab337b 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -54,6 +54,17 @@ func TestRedirectWithExistingGrant(t *testing.T) { assert.Truef(t, len(u.Query().Get("code")) > 30, "authorization code '%s' should be longer then 30", u.Query().Get("code")) } +func TestAuthorizePKCERequiredForPublicClient(t *testing.T) { + defer tests.PrepareTestEnv(t)() + req := NewRequest(t, "GET", "/login/oauth/authorize?client_id=ce5a1322-42a7-11ed-b878-0242ac120002&redirect_uri=http%3A%2F%2F127.0.0.1&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, "invalid_request", u.Query().Get("error")) + assert.Equal(t, "PKCE is required for public clients", u.Query().Get("error_description")) +} + func TestAccessTokenExchange(t *testing.T) { defer tests.PrepareTestEnv(t)() req := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ @@ -168,31 +179,6 @@ func TestAccessTokenExchangeWithInvalidCredentials(t *testing.T) { MakeRequest(t, req, http.StatusBadRequest) } -func TestAccessTokenExchangeForPublicClient(t *testing.T) { - defer tests.PrepareTestEnv(t)() - req := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ - "grant_type": "authorization_code", - "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", - "client_secret": "4MK8Na6R55smdCY0WuCCumZ6hjRPnGY5saWVRHHjJiA=", - // redirect port may vary - "redirect_uri": "http://127.0.0.1:3456", - "code": "authcodepublic", - "code_verifier": "N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt", - }) - MakeRequest(t, req, http.StatusOK) - - req = NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ - "grant_type": "authorization_code", - "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", - "client_secret": "4MK8Na6R55smdCY0WuCCumZ6hjRPnGY5saWVRHHjJiA=", - // redirect port may vary - "redirect_uri": "http://127.0.0.1:3456", - "code": "authcodepublic", - // omit PKCE - }) - MakeRequest(t, req, http.StatusBadRequest) -} - func TestAccessTokenExchangeWithBasicAuth(t *testing.T) { defer tests.PrepareTestEnv(t)() req := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ From f56dd53f97a8bae20314800ee2f26a142dfa305c Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 11 Oct 2022 12:07:01 +0800 Subject: [PATCH 05/11] fix checkbox --- templates/user/settings/applications_oauth2_edit_form.tmpl | 6 +++--- templates/user/settings/applications_oauth2_list.tmpl | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/templates/user/settings/applications_oauth2_edit_form.tmpl b/templates/user/settings/applications_oauth2_edit_form.tmpl index 207dee77f44fb..9ce9d0b56711d 100644 --- a/templates/user/settings/applications_oauth2_edit_form.tmpl +++ b/templates/user/settings/applications_oauth2_edit_form.tmpl @@ -39,9 +39,9 @@
-
- - +
+ +
diff --git a/templates/user/settings/applications_oauth2_list.tmpl b/templates/user/settings/applications_oauth2_list.tmpl index 1fde1c7b7d669..795996f22555c 100644 --- a/templates/user/settings/applications_oauth2_list.tmpl +++ b/templates/user/settings/applications_oauth2_list.tmpl @@ -33,9 +33,9 @@
-
- - +
+ +
From a87a209592375c457c664d37f8f5c26b3d2a9858 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 11 Oct 2022 12:11:16 +0800 Subject: [PATCH 06/11] use Confidential Client instead of Confidential on UI --- options/locale/locale_en-US.ini | 2 +- templates/user/settings/applications_oauth2_edit_form.tmpl | 2 +- templates/user/settings/applications_oauth2_list.tmpl | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 5277255492db4..131a503176069 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -739,7 +739,7 @@ create_oauth2_application_button = Create Application create_oauth2_application_success = You've successfully created a new OAuth2 application. update_oauth2_application_success = You've successfully updated the OAuth2 application. oauth2_application_name = Application Name -oauth2_confidential = Confidential. Select for apps that keep the secret confidential, such as web apps. Do not select for native apps including desktop and mobile apps. +oauth2_confidential_client = Confidential Client. Select for apps that keep the secret confidential, such as web apps. Do not select for native apps including desktop and mobile apps. oauth2_redirect_uri = Redirect URI save_application = Save oauth2_client_id = Client ID diff --git a/templates/user/settings/applications_oauth2_edit_form.tmpl b/templates/user/settings/applications_oauth2_edit_form.tmpl index 9ce9d0b56711d..3baf0e62233c6 100644 --- a/templates/user/settings/applications_oauth2_edit_form.tmpl +++ b/templates/user/settings/applications_oauth2_edit_form.tmpl @@ -40,7 +40,7 @@
- +
diff --git a/templates/user/settings/applications_oauth2_list.tmpl b/templates/user/settings/applications_oauth2_list.tmpl index 795996f22555c..f0003fc4559de 100644 --- a/templates/user/settings/applications_oauth2_list.tmpl +++ b/templates/user/settings/applications_oauth2_list.tmpl @@ -34,7 +34,7 @@
- +
From 918472d7ffd6152a36b01972373e955bea9d1ce7 Mon Sep 17 00:00:00 2001 From: M Hickford Date: Tue, 11 Oct 2022 10:01:45 +0100 Subject: [PATCH 07/11] Address code review comments --- models/migrations/v227.go | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/models/migrations/v227.go b/models/migrations/v227.go index 666c86cd1b32e..756b95819c717 100644 --- a/models/migrations/v227.go +++ b/models/migrations/v227.go @@ -5,22 +5,13 @@ package migrations import ( - "code.gitea.io/gitea/modules/timeutil" "xorm.io/xorm" ) // addConfidentialColumnToOAuth2ApplicationTable: add Confidential column, setting existing rows to true func addConfidentialColumnToOAuth2ApplicationTable(x *xorm.Engine) error { type OAuth2Application struct { - ID int64 `xorm:"pk autoincr"` - UID int64 `xorm:"INDEX"` - Name string - ClientID string `xorm:"unique"` - ClientSecret string - Confidential bool `xorm:"NOT NULL DEFAULT TRUE"` - RedirectURIs []string `xorm:"redirect_uris JSON TEXT"` - CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` - UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` + Confidential bool `xorm:"NOT NULL DEFAULT TRUE"` } return x.Sync(new(OAuth2Application)) From 11326912477cfa4b8544f286b52ce4fd9b78903d Mon Sep 17 00:00:00 2001 From: M Hickford Date: Tue, 11 Oct 2022 18:41:37 +0100 Subject: [PATCH 08/11] Rename Confidential field to ConfidentialClient --- models/auth/oauth2.go | 42 +++++++++---------- models/auth/oauth2_test.go | 4 +- models/fixtures/oauth2_application.yml | 4 +- models/migrations/migrations.go | 2 +- models/migrations/v227.go | 6 +-- modules/convert/convert.go | 14 +++---- modules/structs/user_app.go | 20 ++++----- routers/api/v1/user/app.go | 18 ++++---- routers/web/auth/oauth.go | 2 +- routers/web/user/setting/oauth2_common.go | 18 ++++---- services/forms/user_form.go | 6 +-- templates/swagger/v1_json.tmpl | 8 ++-- .../applications_oauth2_edit_form.tmpl | 4 +- .../settings/applications_oauth2_list.tmpl | 4 +- tests/integration/api_oauth2_apps_test.go | 16 +++---- 15 files changed, 84 insertions(+), 84 deletions(-) diff --git a/models/auth/oauth2.go b/models/auth/oauth2.go index 5913e5335da80..a906c9cca9f39 100644 --- a/models/auth/oauth2.go +++ b/models/auth/oauth2.go @@ -35,10 +35,10 @@ type OAuth2Application struct { // https://datatracker.ietf.org/doc/html/rfc6749#section-2.1 // "Authorization servers MUST record the client type in the client registration details" // https://datatracker.ietf.org/doc/html/rfc8252#section-8.4 - Confidential bool `xorm:"NOT NULL DEFAULT TRUE"` - RedirectURIs []string `xorm:"redirect_uris JSON TEXT"` - CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` - UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` + ConfidentialClient bool `xorm:"NOT NULL DEFAULT TRUE"` + RedirectURIs []string `xorm:"redirect_uris JSON TEXT"` + CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` + UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` } func init() { @@ -62,7 +62,7 @@ func (app *OAuth2Application) PrimaryRedirectURI() string { // ContainsRedirectURI checks if redirectURI is allowed for app func (app *OAuth2Application) ContainsRedirectURI(redirectURI string) bool { - if !app.Confidential { + if !app.ConfidentialClient { uri, err := url.Parse(redirectURI) // ignore port for http loopback uris following https://datatracker.ietf.org/doc/html/rfc8252#section-7.3 if err == nil && uri.Scheme == "http" && uri.Port() != "" { @@ -168,21 +168,21 @@ func GetOAuth2ApplicationsByUserID(ctx context.Context, userID int64) (apps []*O // CreateOAuth2ApplicationOptions holds options to create an oauth2 application type CreateOAuth2ApplicationOptions struct { - Name string - UserID int64 - Confidential bool - RedirectURIs []string + Name string + UserID int64 + ConfidentialClient bool + RedirectURIs []string } // CreateOAuth2Application inserts a new oauth2 application func CreateOAuth2Application(ctx context.Context, opts CreateOAuth2ApplicationOptions) (*OAuth2Application, error) { clientID := uuid.New().String() app := &OAuth2Application{ - UID: opts.UserID, - Name: opts.Name, - ClientID: clientID, - RedirectURIs: opts.RedirectURIs, - Confidential: opts.Confidential, + UID: opts.UserID, + Name: opts.Name, + ClientID: clientID, + RedirectURIs: opts.RedirectURIs, + ConfidentialClient: opts.ConfidentialClient, } if err := db.Insert(ctx, app); err != nil { return nil, err @@ -192,11 +192,11 @@ func CreateOAuth2Application(ctx context.Context, opts CreateOAuth2ApplicationOp // UpdateOAuth2ApplicationOptions holds options to update an oauth2 application type UpdateOAuth2ApplicationOptions struct { - ID int64 - Name string - UserID int64 - Confidential bool - RedirectURIs []string + ID int64 + Name string + UserID int64 + ConfidentialClient bool + RedirectURIs []string } // UpdateOAuth2Application updates an oauth2 application @@ -217,7 +217,7 @@ func UpdateOAuth2Application(opts UpdateOAuth2ApplicationOptions) (*OAuth2Applic app.Name = opts.Name app.RedirectURIs = opts.RedirectURIs - app.Confidential = opts.Confidential + app.ConfidentialClient = opts.ConfidentialClient if err = updateOAuth2Application(ctx, app); err != nil { return nil, err @@ -228,7 +228,7 @@ func UpdateOAuth2Application(opts UpdateOAuth2ApplicationOptions) (*OAuth2Applic } func updateOAuth2Application(ctx context.Context, app *OAuth2Application) error { - if _, err := db.GetEngine(ctx).ID(app.ID).UseBool("Confidential").Update(app); err != nil { + if _, err := db.GetEngine(ctx).ID(app.ID).UseBool("ConfidentialClient").Update(app); err != nil { return err } return nil diff --git a/models/auth/oauth2_test.go b/models/auth/oauth2_test.go index 5bef2edd9f8f7..7a4df6b9acd7a 100644 --- a/models/auth/oauth2_test.go +++ b/models/auth/oauth2_test.go @@ -45,8 +45,8 @@ func TestOAuth2Application_ContainsRedirectURI(t *testing.T) { func TestOAuth2Application_ContainsRedirectURI_WithPort(t *testing.T) { app := &auth_model.OAuth2Application{ - RedirectURIs: []string{"http://127.0.0.1/", "http://::1/", "http://192.168.0.1/", "http://intranet/", "https://127.0.0.1/"}, - Confidential: false, + RedirectURIs: []string{"http://127.0.0.1/", "http://::1/", "http://192.168.0.1/", "http://intranet/", "https://127.0.0.1/"}, + ConfidentialClient: false, } // http loopback uris should ignore port diff --git a/models/fixtures/oauth2_application.yml b/models/fixtures/oauth2_application.yml index bea7b1002e19d..a08b862324e0f 100644 --- a/models/fixtures/oauth2_application.yml +++ b/models/fixtures/oauth2_application.yml @@ -7,7 +7,7 @@ redirect_uris: '["a"]' created_unix: 1546869730 updated_unix: 1546869730 - confidential: true + confidential_client: true - id: 2 uid: 2 @@ -17,4 +17,4 @@ redirect_uris: '["http://127.0.0.1"]' created_unix: 1546869730 updated_unix: 1546869730 - confidential: false + confidential_client: false diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index c4a94f73b328a..76d28eea412cd 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -416,7 +416,7 @@ var migrations = []Migration{ // v226 -> v227 NewMigration("Conan and generic packages do not need to be semantically versioned", fixPackageSemverField), // v227 -> v228 - NewMigration("Add confidential column default true to OAuth2Application table", addConfidentialColumnToOAuth2ApplicationTable), + NewMigration("Add ConfidentialClient column (default true) to OAuth2Application table", addConfidentialClientColumnToOAuth2ApplicationTable), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v227.go b/models/migrations/v227.go index 756b95819c717..f08e6a37641fc 100644 --- a/models/migrations/v227.go +++ b/models/migrations/v227.go @@ -8,10 +8,10 @@ import ( "xorm.io/xorm" ) -// addConfidentialColumnToOAuth2ApplicationTable: add Confidential column, setting existing rows to true -func addConfidentialColumnToOAuth2ApplicationTable(x *xorm.Engine) error { +// addConfidentialColumnToOAuth2ApplicationTable: add ConfidentialClient column, setting existing rows to true +func addConfidentialClientColumnToOAuth2ApplicationTable(x *xorm.Engine) error { type OAuth2Application struct { - Confidential bool `xorm:"NOT NULL DEFAULT TRUE"` + ConfidentialClient bool `xorm:"NOT NULL DEFAULT TRUE"` } return x.Sync(new(OAuth2Application)) diff --git a/modules/convert/convert.go b/modules/convert/convert.go index ffe391dbbd489..5d83c526fdac1 100644 --- a/modules/convert/convert.go +++ b/modules/convert/convert.go @@ -392,13 +392,13 @@ func ToTopicResponse(topic *repo_model.Topic) *api.TopicResponse { // ToOAuth2Application convert from auth.OAuth2Application to api.OAuth2Application func ToOAuth2Application(app *auth.OAuth2Application) *api.OAuth2Application { return &api.OAuth2Application{ - ID: app.ID, - Name: app.Name, - ClientID: app.ClientID, - ClientSecret: app.ClientSecret, - Confidential: app.Confidential, - RedirectURIs: app.RedirectURIs, - Created: app.CreatedUnix.AsTime(), + ID: app.ID, + Name: app.Name, + ClientID: app.ClientID, + ClientSecret: app.ClientSecret, + ConfidentialClient: app.ConfidentialClient, + RedirectURIs: app.RedirectURIs, + Created: app.CreatedUnix.AsTime(), } } diff --git a/modules/structs/user_app.go b/modules/structs/user_app.go index 2614e5bb46082..4cfa5538c8fcc 100644 --- a/modules/structs/user_app.go +++ b/modules/structs/user_app.go @@ -30,21 +30,21 @@ type CreateAccessTokenOption struct { // CreateOAuth2ApplicationOptions holds options to create an oauth2 application type CreateOAuth2ApplicationOptions struct { - Name string `json:"name" binding:"Required"` - Confidential bool `json:"confidential"` - RedirectURIs []string `json:"redirect_uris" binding:"Required"` + Name string `json:"name" binding:"Required"` + ConfidentialClient bool `json:"confidential_client"` + RedirectURIs []string `json:"redirect_uris" binding:"Required"` } // OAuth2Application represents an OAuth2 application. // swagger:response OAuth2Application type OAuth2Application struct { - ID int64 `json:"id"` - Name string `json:"name"` - ClientID string `json:"client_id"` - ClientSecret string `json:"client_secret"` - Confidential bool `json:"confidential"` - RedirectURIs []string `json:"redirect_uris"` - Created time.Time `json:"created"` + ID int64 `json:"id"` + Name string `json:"name"` + ClientID string `json:"client_id"` + ClientSecret string `json:"client_secret"` + ConfidentialClient bool `json:"confidential_client"` + RedirectURIs []string `json:"redirect_uris"` + Created time.Time `json:"created"` } // OAuth2ApplicationList represents a list of OAuth2 applications. diff --git a/routers/api/v1/user/app.go b/routers/api/v1/user/app.go index fa16c2f22ab4b..14f1592591864 100644 --- a/routers/api/v1/user/app.go +++ b/routers/api/v1/user/app.go @@ -213,10 +213,10 @@ func CreateOauth2Application(ctx *context.APIContext) { data := web.GetForm(ctx).(*api.CreateOAuth2ApplicationOptions) app, err := auth_model.CreateOAuth2Application(ctx, auth_model.CreateOAuth2ApplicationOptions{ - Name: data.Name, - UserID: ctx.Doer.ID, - RedirectURIs: data.RedirectURIs, - Confidential: data.Confidential, + Name: data.Name, + UserID: ctx.Doer.ID, + RedirectURIs: data.RedirectURIs, + ConfidentialClient: data.ConfidentialClient, }) if err != nil { ctx.Error(http.StatusBadRequest, "", "error creating oauth2 application") @@ -364,11 +364,11 @@ func UpdateOauth2Application(ctx *context.APIContext) { data := web.GetForm(ctx).(*api.CreateOAuth2ApplicationOptions) app, err := auth_model.UpdateOAuth2Application(auth_model.UpdateOAuth2ApplicationOptions{ - Name: data.Name, - UserID: ctx.Doer.ID, - ID: appID, - RedirectURIs: data.RedirectURIs, - Confidential: data.Confidential, + Name: data.Name, + UserID: ctx.Doer.ID, + ID: appID, + RedirectURIs: data.RedirectURIs, + ConfidentialClient: data.ConfidentialClient, }) if err != nil { if auth_model.IsErrOauthClientIDInvalid(err) || auth_model.IsErrOAuthApplicationNotFound(err) { diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index a0caed985159b..a0a970d130439 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -432,7 +432,7 @@ func AuthorizeOAuth(ctx *context.Context) { case "": // "Authorization servers SHOULD reject authorization requests from native apps that don't use PKCE by returning an error message" // https://datatracker.ietf.org/doc/html/rfc8252#section-8.1 - if !app.Confidential { + if !app.ConfidentialClient { // "the authorization endpoint MUST return the authorization error response with the "error" value set to "invalid_request"" // https://datatracker.ietf.org/doc/html/rfc7636#section-4.4.1 handleAuthorizeError(ctx, AuthorizeError{ diff --git a/routers/web/user/setting/oauth2_common.go b/routers/web/user/setting/oauth2_common.go index 4b126b8edfadd..49ee5c7c2fd82 100644 --- a/routers/web/user/setting/oauth2_common.go +++ b/routers/web/user/setting/oauth2_common.go @@ -39,10 +39,10 @@ func (oa *OAuth2CommonHandlers) AddApp(ctx *context.Context) { // TODO validate redirect URI app, err := auth.CreateOAuth2Application(ctx, auth.CreateOAuth2ApplicationOptions{ - Name: form.Name, - RedirectURIs: []string{form.RedirectURI}, - UserID: oa.OwnerID, - Confidential: form.Confidential, + Name: form.Name, + RedirectURIs: []string{form.RedirectURI}, + UserID: oa.OwnerID, + ConfidentialClient: form.ConfidentialClient, }) if err != nil { ctx.ServerError("CreateOAuth2Application", err) @@ -91,11 +91,11 @@ func (oa *OAuth2CommonHandlers) EditSave(ctx *context.Context) { // TODO validate redirect URI var err error if ctx.Data["App"], err = auth.UpdateOAuth2Application(auth.UpdateOAuth2ApplicationOptions{ - ID: ctx.ParamsInt64("id"), - Name: form.Name, - RedirectURIs: []string{form.RedirectURI}, - UserID: oa.OwnerID, - Confidential: form.Confidential, + ID: ctx.ParamsInt64("id"), + Name: form.Name, + RedirectURIs: []string{form.RedirectURI}, + UserID: oa.OwnerID, + ConfidentialClient: form.ConfidentialClient, }); err != nil { ctx.ServerError("UpdateOAuth2Application", err) return diff --git a/services/forms/user_form.go b/services/forms/user_form.go index 9c9ebc09ce16b..036c2ca3ec2c5 100644 --- a/services/forms/user_form.go +++ b/services/forms/user_form.go @@ -379,9 +379,9 @@ func (f *NewAccessTokenForm) Validate(req *http.Request, errs binding.Errors) bi // EditOAuth2ApplicationForm form for editing oauth2 applications type EditOAuth2ApplicationForm struct { - Name string `binding:"Required;MaxSize(255)" form:"application_name"` - RedirectURI string `binding:"Required" form:"redirect_uri"` - Confidential bool `form:"confidential"` + Name string `binding:"Required;MaxSize(255)" form:"application_name"` + RedirectURI string `binding:"Required" form:"redirect_uri"` + ConfidentialClient bool `form:"confidential_client"` } // Validate validates the fields diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 2b0b585df11c8..94fb67ab44568 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -14645,9 +14645,9 @@ "description": "CreateOAuth2ApplicationOptions holds options to create an oauth2 application", "type": "object", "properties": { - "confidential": { + "confidential_client": { "type": "boolean", - "x-go-name": "Confidential" + "x-go-name": "ConfidentialClient" }, "name": { "type": "string", @@ -17310,9 +17310,9 @@ "type": "string", "x-go-name": "ClientSecret" }, - "confidential": { + "confidential_client": { "type": "boolean", - "x-go-name": "Confidential" + "x-go-name": "ConfidentialClient" }, "created": { "type": "string", diff --git a/templates/user/settings/applications_oauth2_edit_form.tmpl b/templates/user/settings/applications_oauth2_edit_form.tmpl index 3baf0e62233c6..bedf9789ac496 100644 --- a/templates/user/settings/applications_oauth2_edit_form.tmpl +++ b/templates/user/settings/applications_oauth2_edit_form.tmpl @@ -39,9 +39,9 @@
-
+
- +
diff --git a/templates/user/settings/applications_oauth2_list.tmpl b/templates/user/settings/applications_oauth2_list.tmpl index f0003fc4559de..ba9b3aa148c4d 100644 --- a/templates/user/settings/applications_oauth2_list.tmpl +++ b/templates/user/settings/applications_oauth2_list.tmpl @@ -33,9 +33,9 @@
-
+
- +
diff --git a/tests/integration/api_oauth2_apps_test.go b/tests/integration/api_oauth2_apps_test.go index 2336d130c6744..6352449d6acb1 100644 --- a/tests/integration/api_oauth2_apps_test.go +++ b/tests/integration/api_oauth2_apps_test.go @@ -34,7 +34,7 @@ func testAPICreateOAuth2Application(t *testing.T) { RedirectURIs: []string{ "http://www.google.com", }, - Confidential: true, + ConfidentialClient: true, } req := NewRequestWithJSON(t, "POST", "/api/v1/user/applications/oauth2", &appBody) @@ -47,7 +47,7 @@ func testAPICreateOAuth2Application(t *testing.T) { assert.EqualValues(t, appBody.Name, createdApp.Name) assert.Len(t, createdApp.ClientSecret, 56) assert.Len(t, createdApp.ClientID, 36) - assert.True(t, createdApp.Confidential) + assert.True(t, createdApp.ConfidentialClient) assert.NotEmpty(t, createdApp.Created) assert.EqualValues(t, appBody.RedirectURIs[0], createdApp.RedirectURIs[0]) unittest.AssertExistsAndLoadBean(t, &auth.OAuth2Application{UID: user.ID, Name: createdApp.Name}) @@ -64,7 +64,7 @@ func testAPIListOAuth2Applications(t *testing.T) { RedirectURIs: []string{ "http://www.google.com", }, - Confidential: true, + ConfidentialClient: true, }) urlStr := fmt.Sprintf("/api/v1/user/applications/oauth2?token=%s", token) @@ -77,7 +77,7 @@ func testAPIListOAuth2Applications(t *testing.T) { assert.EqualValues(t, existApp.Name, expectedApp.Name) assert.EqualValues(t, existApp.ClientID, expectedApp.ClientID) - assert.Equal(t, existApp.Confidential, expectedApp.Confidential) + assert.Equal(t, existApp.ConfidentialClient, expectedApp.ConfidentialClient) assert.Len(t, expectedApp.ClientID, 36) assert.Empty(t, expectedApp.ClientSecret) assert.EqualValues(t, existApp.RedirectURIs[0], expectedApp.RedirectURIs[0]) @@ -116,7 +116,7 @@ func testAPIGetOAuth2Application(t *testing.T) { RedirectURIs: []string{ "http://www.google.com", }, - Confidential: true, + ConfidentialClient: true, }) urlStr := fmt.Sprintf("/api/v1/user/applications/oauth2/%d?token=%s", existApp.ID, token) @@ -129,7 +129,7 @@ func testAPIGetOAuth2Application(t *testing.T) { assert.EqualValues(t, existApp.Name, expectedApp.Name) assert.EqualValues(t, existApp.ClientID, expectedApp.ClientID) - assert.Equal(t, existApp.Confidential, expectedApp.Confidential) + assert.Equal(t, existApp.ConfidentialClient, expectedApp.ConfidentialClient) assert.Len(t, expectedApp.ClientID, 36) assert.Empty(t, expectedApp.ClientSecret) assert.Len(t, expectedApp.RedirectURIs, 1) @@ -154,7 +154,7 @@ func testAPIUpdateOAuth2Application(t *testing.T) { "http://www.google.com/", "http://www.github.com/", }, - Confidential: true, + ConfidentialClient: true, } urlStr := fmt.Sprintf("/api/v1/user/applications/oauth2/%d", existApp.ID) @@ -169,6 +169,6 @@ func testAPIUpdateOAuth2Application(t *testing.T) { assert.Len(t, expectedApp.RedirectURIs, 2) assert.EqualValues(t, expectedApp.RedirectURIs[0], appBody.RedirectURIs[0]) assert.EqualValues(t, expectedApp.RedirectURIs[1], appBody.RedirectURIs[1]) - assert.Equal(t, expectedApp.Confidential, appBody.Confidential) + assert.Equal(t, expectedApp.ConfidentialClient, appBody.ConfidentialClient) unittest.AssertExistsAndLoadBean(t, &auth.OAuth2Application{ID: expectedApp.ID, Name: expectedApp.Name}) } From 89626e2725c37cfb4e8fec30aadf4e9132a02647 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 12 Oct 2022 10:53:18 +0800 Subject: [PATCH 09/11] fix xrom update --- models/auth/oauth2.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/auth/oauth2.go b/models/auth/oauth2.go index a906c9cca9f39..03ee9025f281f 100644 --- a/models/auth/oauth2.go +++ b/models/auth/oauth2.go @@ -228,7 +228,7 @@ func UpdateOAuth2Application(opts UpdateOAuth2ApplicationOptions) (*OAuth2Applic } func updateOAuth2Application(ctx context.Context, app *OAuth2Application) error { - if _, err := db.GetEngine(ctx).ID(app.ID).UseBool("ConfidentialClient").Update(app); err != nil { + if _, err := db.GetEngine(ctx).ID(app.ID).UseBool("confidential_client").Update(app); err != nil { return err } return nil From 28bb6e806ec9b1ed687d1b44baca71deed8f5197 Mon Sep 17 00:00:00 2001 From: M Hickford Date: Sun, 23 Oct 2022 22:25:37 +0100 Subject: [PATCH 10/11] move confidential client checkbox under redirect uri --- .../user/settings/applications_oauth2_edit_form.tmpl | 8 ++++---- templates/user/settings/applications_oauth2_list.tmpl | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/templates/user/settings/applications_oauth2_edit_form.tmpl b/templates/user/settings/applications_oauth2_edit_form.tmpl index bedf9789ac496..9d7273fd6d0bf 100644 --- a/templates/user/settings/applications_oauth2_edit_form.tmpl +++ b/templates/user/settings/applications_oauth2_edit_form.tmpl @@ -39,14 +39,14 @@
-
- - -
+
+ + +
diff --git a/templates/user/settings/applications_oauth2_list.tmpl b/templates/user/settings/applications_oauth2_list.tmpl index ba9b3aa148c4d..fbca5934cd572 100644 --- a/templates/user/settings/applications_oauth2_list.tmpl +++ b/templates/user/settings/applications_oauth2_list.tmpl @@ -33,14 +33,14 @@
-
- - -
+
+ + +
From 1274ae0fbe1b495d637bf2b944a11b0620af6846 Mon Sep 17 00:00:00 2001 From: M Hickford Date: Mon, 24 Oct 2022 03:27:41 +0100 Subject: [PATCH 11/11] test migration defaults column to true --- .../o_auth2_application.yml | 2 + models/migrations/v230_test.go | 46 +++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 models/migrations/fixtures/Test_addConfidentialClientColumnToOAuth2ApplicationTable/o_auth2_application.yml create mode 100644 models/migrations/v230_test.go diff --git a/models/migrations/fixtures/Test_addConfidentialClientColumnToOAuth2ApplicationTable/o_auth2_application.yml b/models/migrations/fixtures/Test_addConfidentialClientColumnToOAuth2ApplicationTable/o_auth2_application.yml new file mode 100644 index 0000000000000..a88c2ef89f088 --- /dev/null +++ b/models/migrations/fixtures/Test_addConfidentialClientColumnToOAuth2ApplicationTable/o_auth2_application.yml @@ -0,0 +1,2 @@ +- + id: 1 diff --git a/models/migrations/v230_test.go b/models/migrations/v230_test.go new file mode 100644 index 0000000000000..98ba3f5d97209 --- /dev/null +++ b/models/migrations/v230_test.go @@ -0,0 +1,46 @@ +// Copyright 2022 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func Test_addConfidentialClientColumnToOAuth2ApplicationTable(t *testing.T) { + // premigration + type OAuth2Application struct { + ID int64 + } + + // Prepare and load the testing database + x, deferable := prepareTestEnv(t, 0, new(OAuth2Application)) + defer deferable() + if x == nil || t.Failed() { + return + } + + if err := addConfidentialClientColumnToOAuth2ApplicationTable(x); err != nil { + assert.NoError(t, err) + return + } + + // postmigration + type ExpectedOAuth2Application struct { + ID int64 + ConfidentialClient bool + } + + got := []ExpectedOAuth2Application{} + if err := x.Table("o_auth2_application").Select("id, confidential_client").Find(&got); !assert.NoError(t, err) { + return + } + + assert.NotEmpty(t, got) + for _, e := range got { + assert.True(t, e.ConfidentialClient) + } +}