diff --git a/services/graph/pkg/config/config.go b/services/graph/pkg/config/config.go index 5b2f00e5189..dc90b8ef4bd 100644 --- a/services/graph/pkg/config/config.go +++ b/services/graph/pkg/config/config.go @@ -92,7 +92,8 @@ type Identity struct { // API represents API configuration parameters. type API struct { - GroupMembersPatchLimit int `yaml:"group_members_patch_limit" env:"GRAPH_GROUP_MEMBERS_PATCH_LIMIT" desc:"The amount of group members allowed to be added with a single patch request."` + GroupMembersPatchLimit int `yaml:"group_members_patch_limit" env:"GRAPH_GROUP_MEMBERS_PATCH_LIMIT" desc:"The amount of group members allowed to be added with a single patch request."` + UsernameMatch string `yaml:"graph_username_match" env:"GRAPH_USERNAME_MATCH" desc:"Option to allow legacy usernames. Supported options are 'default' and 'none'."` } // Events combines the configuration options for the event bus. diff --git a/services/graph/pkg/config/defaults/defaultconfig.go b/services/graph/pkg/config/defaults/defaultconfig.go index 023efe04939..1c494566ef1 100644 --- a/services/graph/pkg/config/defaults/defaultconfig.go +++ b/services/graph/pkg/config/defaults/defaultconfig.go @@ -36,6 +36,7 @@ func DefaultConfig() *config.Config { }, API: config.API{ GroupMembersPatchLimit: 20, + UsernameMatch: "default", }, Reva: shared.DefaultRevaConfig(), Spaces: config.Spaces{ diff --git a/services/graph/pkg/config/parser/parse.go b/services/graph/pkg/config/parser/parse.go index ffd91247ea8..6f48d5c4833 100644 --- a/services/graph/pkg/config/parser/parse.go +++ b/services/graph/pkg/config/parser/parse.go @@ -52,5 +52,15 @@ func Validate(cfg *config.Config) error { "graph", defaults2.BaseConfigPath()) } + switch cfg.API.UsernameMatch { + case "default", "none": + default: + return fmt.Errorf("The username match validator is invalid for %s. "+ + "Make sure your %s config contains the proper values "+ + "(e.g. by running ocis init or setting it manually in "+ + "the config/corresponding environment variable).", + "graph", defaults2.BaseConfigPath()) + } + return nil } diff --git a/services/graph/pkg/service/v0/educationuser.go b/services/graph/pkg/service/v0/educationuser.go index 611a3d38935..8a5cdb3c724 100644 --- a/services/graph/pkg/service/v0/educationuser.go +++ b/services/graph/pkg/service/v0/educationuser.go @@ -112,7 +112,7 @@ func (g Graph) PostEducationUser(w http.ResponseWriter, r *http.Request) { } if accountName, ok := u.GetOnPremisesSamAccountNameOk(); ok { - if !isValidUsername(*accountName) { + if !g.isValidUsername(*accountName) { logger.Debug().Str("username", *accountName).Msg("could not create education user: username must be at least the local part of an email") errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, fmt.Sprintf("username %s must be at least the local part of an email", *u.OnPremisesSamAccountName)) return diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go index b37180b12d5..201d64af177 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/services/graph/pkg/service/v0/users.go @@ -159,7 +159,7 @@ func (g Graph) PostUser(w http.ResponseWriter, r *http.Request) { return } if accountName, ok := u.GetOnPremisesSamAccountNameOk(); ok { - if !isValidUsername(*accountName) { + if !g.isValidUsername(*accountName) { logger.Debug().Str("username", *accountName).Msg("could not create user: username must be at least the local part of an email") errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, fmt.Sprintf("username %s must be at least the local part of an email", *u.OnPremisesSamAccountName)) return @@ -530,16 +530,28 @@ func (g Graph) PatchUser(w http.ResponseWriter, r *http.Request) { } -// We want to allow email addresses as usernames so they show up when using them in ACLs on storages that allow integration with our glauth LDAP service -// so we are adding a few restrictions from https://stackoverflow.com/questions/6949667/what-are-the-real-rules-for-linux-usernames-on-centos-6-and-rhel-6 -// names should not start with numbers -var usernameRegex = regexp.MustCompile("^[a-zA-Z_][a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]*(@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*)*$") +const ( + usernameMatchDefault = "default" + usernameMatchNone = "none" +) + +var usernameRegexes = map[string]*regexp.Regexp{ + // We want to allow email addresses as usernames so they show up when using them in ACLs on storages that allow integration with our glauth LDAP service + // so we are adding a few restrictions from https://stackoverflow.com/questions/6949667/what-are-the-real-rules-for-linux-usernames-on-centos-6-and-rhel-6 + // names should not start with numbers + usernameMatchDefault: regexp.MustCompile("^[a-zA-Z_][a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]*(@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*)*$"), -func isValidUsername(e string) bool { + // In some cases users will be provisioned from an existing system, which may or may not have strange usernames. Because of this we want to "trust" the + // upstream system and allow weird usernames, so relying on the used identity provider to complain if a username is violating its restrictions. + usernameMatchNone: regexp.MustCompile(".*"), +} + +func (g Graph) isValidUsername(e string) bool { if len(e) < 1 && len(e) > 254 { return false } - return usernameRegex.MatchString(e) + + return usernameRegexes[g.config.API.UsernameMatch].MatchString(e) } // regex from https://www.w3.org/TR/2016/REC-html51-20161101/sec-forms.html#valid-e-mail-address diff --git a/services/graph/pkg/service/v0/users_test.go b/services/graph/pkg/service/v0/users_test.go index 34958e9af6c..d516782534b 100644 --- a/services/graph/pkg/service/v0/users_test.go +++ b/services/graph/pkg/service/v0/users_test.go @@ -539,6 +539,63 @@ var _ = Describe("Users", func() { Expect(rr.Code).To(Equal(http.StatusOK)) }) + + Describe("Handling usernames with spaces", func() { + var ( + newSvc = func(usernameMatch string) service.Service { + localCfg := defaults.FullDefaultConfig() + localCfg.Identity.LDAP.CACert = "" // skip the startup checks, we don't use LDAP at all in this tests + localCfg.TokenManager.JWTSecret = "loremipsum" + localCfg.Commons = &shared.Commons{} + localCfg.GRPCClientTLS = &shared.GRPCClientTLS{} + + localCfg.API.UsernameMatch = usernameMatch + + _ = ogrpc.Configure(ogrpc.GetClientOptions(cfg.GRPCClientTLS)...) + localSvc, _ := service.NewService( + service.Config(localCfg), + service.WithGatewayClient(gatewayClient), + service.EventsPublisher(&eventsPublisher), + service.WithIdentityBackend(identityBackend), + service.WithRoleService(roleService), + ) + + return localSvc + } + ) + + BeforeEach(func() { + user.SetOnPremisesSamAccountName("username with spaces") + }) + + It("rejects a username with spaces if match regex is default", func() { + userJson, err := json.Marshal(user) + Expect(err).ToNot(HaveOccurred()) + + r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/me/users", bytes.NewBuffer(userJson)) + r = r.WithContext(revactx.ContextSetUser(ctx, currentUser)) + + newSvc("default").PostUser(rr, r) + Expect(rr.Code).To(Equal(http.StatusBadRequest)) + }) + + It("creates a user with spaces in username if regex is none", func() { + roleService.On("AssignRoleToUser", mock.Anything, mock.Anything).Return(&settings.AssignRoleToUserResponse{}, nil) + identityBackend.On("CreateUser", mock.Anything, mock.Anything).Return(func(ctx context.Context, user libregraph.User) *libregraph.User { + user.SetId("/users/user") + return &user + }, nil) + userJson, err := json.Marshal(user) + Expect(err).ToNot(HaveOccurred()) + + r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/me/users", bytes.NewBuffer(userJson)) + r = r.WithContext(revactx.ContextSetUser(ctx, currentUser)) + newSvc("none").PostUser(rr, r) + + Expect(rr.Code).To(Equal(http.StatusOK)) + }) + }) + }) Describe("DeleteUser", func() {