Fix issues found in PR #887

Signed-off-by: Nicola Murino <nicola.murino@gmail.com>
2024-11-21 23:20:24 +00:00 · 2022-06-25 10:41:15 +02:00 · 2022-06-25 10:41:15 +02:00 · 9a6b1a1315
commit 9a6b1a1315
parent 90009a649d
8 changed files with 36 additions and 16 deletions
--- a/config/config.go
+++ b/config/config.go
@ -105,6 +105,7 @@ var (
 			UsernameField:   "",
 			RoleField:       "",
 			ImplicitRoles:   false,
+			Scopes:          []string{"openid", "profile", "email"},
 			CustomFields:    []string{},
 		},
 		Security: httpd.SecurityConf{
@ -1337,7 +1338,10 @@ func getHTTPDSecurityConfFromEnv(idx int) (httpd.SecurityConf, bool) { //nolint:
 }

 func getHTTPDOIDCFromEnv(idx int) (httpd.OIDC, bool) {
-	var result httpd.OIDC
+	result := defaultHTTPDBinding.OIDC
+	if len(globalConf.HTTPDConfig.Bindings) > idx {
+		result = globalConf.HTTPDConfig.Bindings[idx].OIDC
+	}
 	isSet := false

 	clientID, ok := os.LookupEnv(fmt.Sprintf("SFTPGO_HTTPD__BINDINGS__%v__OIDC__CLIENT_ID", idx))
@ -1503,12 +1507,7 @@ func getHTTPDWebClientIntegrationsFromEnv(idx int) []httpd.WebClientIntegration
 }

 func getDefaultHTTPBinding(idx int) httpd.Binding {
-	binding := httpd.Binding{
-		EnableWebAdmin:  true,
-		EnableWebClient: true,
-		RenderOpenAPI:   true,
-		MinTLSVersion:   12,
-	}
+	binding := defaultHTTPDBinding
 	if len(globalConf.HTTPDConfig.Bindings) > idx {
 		binding = globalConf.HTTPDConfig.Bindings[idx]
 	}
--- a/config/config_test.go
+++ b/config/config_test.go
@ -915,6 +915,7 @@ func TestHTTPDBindingsFromEnv(t *testing.T) {
 	os.Setenv("SFTPGO_HTTPD__BINDINGS__2__OIDC__REDIRECT_BASE_URL", "redirect base url")
 	os.Setenv("SFTPGO_HTTPD__BINDINGS__2__OIDC__USERNAME_FIELD", "preferred_username")
 	os.Setenv("SFTPGO_HTTPD__BINDINGS__2__OIDC__ROLE_FIELD", "sftpgo_role")
+	os.Setenv("SFTPGO_HTTPD__BINDINGS__2__OIDC__SCOPES", "openid")
 	os.Setenv("SFTPGO_HTTPD__BINDINGS__2__OIDC__IMPLICIT_ROLES", "1")
 	os.Setenv("SFTPGO_HTTPD__BINDINGS__2__OIDC__CUSTOM_FIELDS", "field1,field2")
 	os.Setenv("SFTPGO_HTTPD__BINDINGS__2__SECURITY__ENABLED", "true")
@ -979,6 +980,7 @@ func TestHTTPDBindingsFromEnv(t *testing.T) {
 		os.Unsetenv("SFTPGO_HTTPD__BINDINGS__2__OIDC__REDIRECT_BASE_URL")
 		os.Unsetenv("SFTPGO_HTTPD__BINDINGS__2__OIDC__USERNAME_FIELD")
 		os.Unsetenv("SFTPGO_HTTPD__BINDINGS__2__OIDC__ROLE_FIELD")
+		os.Unsetenv("SFTPGO_HTTPD__BINDINGS__2__OIDC__SCOPES")
 		os.Unsetenv("SFTPGO_HTTPD__BINDINGS__2__OIDC__IMPLICIT_ROLES")
 		os.Unsetenv("SFTPGO_HTTPD__BINDINGS__2__OIDC__CUSTOM_FIELDS")
 		os.Unsetenv("SFTPGO_HTTPD__BINDINGS__2__SECURITY__ENABLED")
@ -1028,6 +1030,7 @@ func TestHTTPDBindingsFromEnv(t *testing.T) {
 	require.Equal(t, 0, bindings[0].HideLoginURL)
 	require.False(t, bindings[0].Security.Enabled)
 	require.Equal(t, 0, bindings[0].ClientIPHeaderDepth)
+	require.Len(t, bindings[0].OIDC.Scopes, 3)
 	require.Equal(t, 8000, bindings[1].Port)
 	require.Equal(t, "127.0.0.1", bindings[1].Address)
 	require.False(t, bindings[1].EnableHTTPS)
@ -1038,6 +1041,7 @@ func TestHTTPDBindingsFromEnv(t *testing.T) {
 	require.Nil(t, bindings[1].TLSCipherSuites)
 	require.Equal(t, 1, bindings[1].HideLoginURL)
 	require.Empty(t, bindings[1].OIDC.ClientID)
+	require.Len(t, bindings[1].OIDC.Scopes, 3)
 	require.False(t, bindings[1].Security.Enabled)
 	require.Equal(t, "Web Admin", bindings[1].Branding.WebAdmin.Name)
 	require.Equal(t, "WebClient", bindings[1].Branding.WebClient.ShortName)
@ -1068,6 +1072,8 @@ func TestHTTPDBindingsFromEnv(t *testing.T) {
 	require.Equal(t, "redirect base url", bindings[2].OIDC.RedirectBaseURL)
 	require.Equal(t, "preferred_username", bindings[2].OIDC.UsernameField)
 	require.Equal(t, "sftpgo_role", bindings[2].OIDC.RoleField)
+	require.Len(t, bindings[2].OIDC.Scopes, 1)
+	require.Equal(t, "openid", bindings[2].OIDC.Scopes[0])
 	require.True(t, bindings[2].OIDC.ImplicitRoles)
 	require.Len(t, bindings[2].OIDC.CustomFields, 2)
 	require.Equal(t, "field1", bindings[2].OIDC.CustomFields[0])
--- a/docs/full-configuration.md
+++ b/docs/full-configuration.md
@ -279,7 +279,7 @@ The configuration file contains the following sections:
      - `client_secret`, string. Defines the application's secret. Default: blank.
      - `redirect_base_url`, string. Defines the base URL to redirect to after OpenID authentication. The suffix `/web/oidc/redirect` will be added to this base URL, adding also the `web_root` if configured. Default: blank.
      - `username_field`, string. Defines the ID token claims field to map to the SFTPGo username. Default: blank.
-      - `scopes`, list of strings. Request the OAuth provider to provide the scope information from an authenticated users. Default: `"openid", "profile", "email"`.
+      - `scopes`, list of strings. Request the OAuth provider to provide the scope information from an authenticated users. The `openid` scope is mandatory. Default: `"openid", "profile", "email"`.
      - `role_field`, string. Defines the optional ID token claims field to map to a SFTPGo role. If the defined ID token claims field is set to `admin` the authenticated user is mapped to an SFTPGo admin. You don't need to specify this field if you want to use OpenID only for the Web Client UI. Default: blank.
      - `implicit_roles`, boolean. If set, the `role_field` is ignored and the SFTPGo role is assumed based on the login link used. Default: `false`.
      - `custom_fields`, list of strings. Custom token claims fields to pass to the pre-login hook. Default: empty.
--- a/go.mod
+++ b/go.mod
@ -68,7 +68,7 @@ require (
 	golang.org/x/crypto v0.0.0-20220622213112-05595931fe9d
 	golang.org/x/net v0.0.0-20220622184535-263ec571b305
 	golang.org/x/oauth2 v0.0.0-20220622183110-fd043fe589d2
-	golang.org/x/sys v0.0.0-20220622161953-175b2fd9d664
+	golang.org/x/sys v0.0.0-20220624220833-87e55d714810
 	golang.org/x/time v0.0.0-20220609170525-579cf78fd858
 	google.golang.org/api v0.85.0
 	gopkg.in/natefinch/lumberjack.v2 v2.0.0
@ -155,7 +155,7 @@ require (
 	golang.org/x/tools v0.1.11 // indirect
 	golang.org/x/xerrors v0.0.0-20220609144429-65e65417b02f // indirect
 	google.golang.org/appengine v1.6.7 // indirect
-	google.golang.org/genproto v0.0.0-20220623142657-077d458a5694 // indirect
+	google.golang.org/genproto v0.0.0-20220624142145-8cd45d7dbd1f // indirect
 	google.golang.org/grpc v1.47.0 // indirect
 	google.golang.org/protobuf v1.28.0 // indirect
 	gopkg.in/ini.v1 v1.66.6 // indirect
--- a/go.sum
+++ b/go.sum
@ -966,8 +966,8 @@ golang.org/x/sys v0.0.0-20220513210249-45d2b4557a2a/go.mod h1:oPkhp1MJrh7nUepCBc
 golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220610221304-9f5ed59c137d/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220615213510-4f61da869c0c/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.0.0-20220622161953-175b2fd9d664 h1:wEZYwx+kK+KlZ0hpvP2Ls1Xr4+RWnlzGFwPP0aiDjIU=
-golang.org/x/sys v0.0.0-20220622161953-175b2fd9d664/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
+golang.org/x/sys v0.0.0-20220624220833-87e55d714810 h1:rHZQSjJdAI4Xf5Qzeh2bBc5YJIkPFVM6oDtMFYmgws0=
+golang.org/x/sys v0.0.0-20220624220833-87e55d714810/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
 golang.org/x/term v0.0.0-20210927222741-03fcf44c2211 h1:JGgROgKl9N8DuW20oFS5gxc+lE67/N3FcwmBPMe7ArY=
 golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8=
@ -1214,8 +1214,8 @@ google.golang.org/genproto v0.0.0-20220523171625-347a074981d8/go.mod h1:RAyBrSAP
 google.golang.org/genproto v0.0.0-20220608133413-ed9918b62aac/go.mod h1:KEWEmljWE5zPzLBa/oHl6DaEt9LmfH6WtH1OHIvleBA=
 google.golang.org/genproto v0.0.0-20220616135557-88e70c0c3a90/go.mod h1:KEWEmljWE5zPzLBa/oHl6DaEt9LmfH6WtH1OHIvleBA=
 google.golang.org/genproto v0.0.0-20220617124728-180714bec0ad/go.mod h1:KEWEmljWE5zPzLBa/oHl6DaEt9LmfH6WtH1OHIvleBA=
-google.golang.org/genproto v0.0.0-20220623142657-077d458a5694 h1:itnFmgk4Ls5nT+mYO2ZK6F0DpKsGZLhB5BB9y5ZL2HA=
-google.golang.org/genproto v0.0.0-20220623142657-077d458a5694/go.mod h1:KEWEmljWE5zPzLBa/oHl6DaEt9LmfH6WtH1OHIvleBA=
+google.golang.org/genproto v0.0.0-20220624142145-8cd45d7dbd1f h1:hJ/Y5SqPXbarffmAsApliUlcvMU+wScNGfyop4bZm8o=
+google.golang.org/genproto v0.0.0-20220624142145-8cd45d7dbd1f/go.mod h1:KEWEmljWE5zPzLBa/oHl6DaEt9LmfH6WtH1OHIvleBA=
 google.golang.org/grpc v1.19.0/go.mod h1:mqu4LbDTu4XGKhr4mRzUsmM4RtVoemTSY81AxZiDr8c=
 google.golang.org/grpc v1.20.1/go.mod h1:10oTOabMzJvdu6/UiuZezV6QK5dSlG84ov/aaiqXj38=
 google.golang.org/grpc v1.21.1/go.mod h1:oYelfM1adQP15Ek0mdvEgi9Df8B9CZIaU1084ijfRaM=
--- a/httpd/oidc.go
+++ b/httpd/oidc.go
@ -70,7 +70,9 @@ type OIDC struct {
 	// If set, the `RoleField` is ignored and the SFTPGo role is assumed based on
 	// the login link used
 	ImplicitRoles bool `json:"implicit_roles" mapstructure:"implicit_roles"`
-	// Scopes required by the OAuth provider to retrieve information about the authenticated user. Refer to your OAuth provider documentation for more information about this
+	// Scopes required by the OAuth provider to retrieve information about the authenticated user.
+	// The "openid" scope is required.
+	// Refer to your OAuth provider documentation for more information about this
 	Scopes []string `json:"scopes" mapstructure:"scopes"`
 	// Custom token claims fields to pass to the pre-login hook
 	CustomFields      []string `json:"custom_fields" mapstructure:"custom_fields"`
@ -118,6 +120,9 @@ func (o *OIDC) initialize() error {
 	if o.RedirectBaseURL == "" {
 		return errors.New("oidc: redirect base URL cannot be empty")
 	}
+	if !util.Contains(o.Scopes, oidc.ScopeOpenID) {
+		return fmt.Errorf("oidc: required scope %q is not set", oidc.ScopeOpenID)
+	}
 	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
 	defer cancel()

--- a/httpd/oidc_test.go
+++ b/httpd/oidc_test.go
@ -93,6 +93,11 @@ func TestOIDCInitialization(t *testing.T) {
 		RoleField:       "sftpgo_role",
 	}
 	err = config.initialize()
+	if assert.Error(t, err) {
+		assert.Contains(t, err.Error(), "oidc: required scope \"openid\" is not set")
+	}
+	config.Scopes = []string{oidc.ScopeOpenID}
+	err = config.initialize()
 	if assert.Error(t, err) {
 		assert.Contains(t, err.Error(), "oidc: unable to initialize provider")
 	}
@ -1263,6 +1268,7 @@ func getTestOIDCServer() *httpdServer {
 				UsernameField:   "preferred_username",
 				RoleField:       "sftpgo_role",
 				ImplicitRoles:   false,
+				Scopes:          []string{oidc.ScopeOpenID, "profile", "email"},
 				CustomFields:    nil,
 			},
 		},
--- a/sftpgo.json
+++ b/sftpgo.json
@ -262,7 +262,11 @@
          "client_secret": "",
          "config_url": "",
          "redirect_base_url": "",
-          "scopes": [ "openid", "profile", "email" ],
+          "scopes": [
+            "openid",
+            "profile",
+            "email"
+          ],
          "username_field": "",
          "role_field": "",
          "implicit_roles": false,