From fe2501b083aee79bf2f80f59c8496ab3a64fe136 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 2 Mar 2022 16:10:20 +0100 Subject: [PATCH 1/2] graph: Add some validation for username and email This copies the validation code from the accounts service, also fixing a bug in the regex that allowed adding mail addresses with whitespace and other problematic characters to the domain part of the mail address. Partial fix for: #3247 --- accounts/pkg/service/v0/accounts.go | 4 +-- graph/pkg/service/v0/users.go | 44 ++++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/accounts/pkg/service/v0/accounts.go b/accounts/pkg/service/v0/accounts.go index 2b393cb710..bd28223106 100644 --- a/accounts/pkg/service/v0/accounts.go +++ b/accounts/pkg/service/v0/accounts.go @@ -746,7 +746,7 @@ func validateAccountEmail(serviceID string, a *accountsmsg.Account) error { // 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])?)*)*$") +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])?)*)*$") func isValidUsername(e string) bool { if len(e) < 1 && len(e) > 254 { @@ -756,7 +756,7 @@ func isValidUsername(e string) bool { } // regex from https://www.w3.org/TR/2016/REC-html51-20161101/sec-forms.html#valid-e-mail-address -var emailRegex = regexp.MustCompile("^[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])?)*$") +var emailRegex = regexp.MustCompile("^[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 isValidEmail(e string) bool { if len(e) < 3 && len(e) > 254 { diff --git a/graph/pkg/service/v0/users.go b/graph/pkg/service/v0/users.go index 88d87a420e..fcce5dc8f2 100644 --- a/graph/pkg/service/v0/users.go +++ b/graph/pkg/service/v0/users.go @@ -3,8 +3,10 @@ package svc import ( "encoding/json" "errors" + "fmt" "net/http" "net/url" + "regexp" revactx "github.com/cs3org/reva/pkg/ctx" "github.com/go-chi/chi/v5" @@ -56,7 +58,18 @@ func (g Graph) PostUser(w http.ResponseWriter, r *http.Request) { } if isNilOrEmpty(u.DisplayName) || isNilOrEmpty(u.OnPremisesSamAccountName) || isNilOrEmpty(u.Mail) { - errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "Missing Required Attribute") + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, err.Error()) + return + } + + if !isValidUsername(*u.OnPremisesSamAccountName) { + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, + fmt.Sprintf("username '%s' must be at least the local part of an email", *u.OnPremisesSamAccountName)) + return + } + if !isValidEmail(*u.Mail) { + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, + fmt.Sprintf("'%s' is not a valid email address", *u.Mail)) return } @@ -151,6 +164,13 @@ func (g Graph) PatchUser(w http.ResponseWriter, r *http.Request) { return } + mail := changes.GetMail() + if !isValidEmail(mail) { + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, + fmt.Sprintf("'%s' is not a valid email address", mail)) + return + } + u, err := g.identityBackend.UpdateUser(r.Context(), nameOrID, *changes) if err != nil { var errcode errorcode.Error @@ -169,3 +189,25 @@ func (g Graph) PatchUser(w http.ResponseWriter, r *http.Request) { func isNilOrEmpty(s *string) bool { return s == nil || *s == "" } + +// 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])?)*)*$") + +func isValidUsername(e string) bool { + if len(e) < 1 && len(e) > 254 { + return false + } + return usernameRegex.MatchString(e) +} + +// regex from https://www.w3.org/TR/2016/REC-html51-20161101/sec-forms.html#valid-e-mail-address +var emailRegex = regexp.MustCompile("^[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 isValidEmail(e string) bool { + if len(e) < 3 && len(e) > 254 { + return false + } + return emailRegex.MatchString(e) +} From 06ca18b1fb93bd8af55562404f5258a8368a499b Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 2 Mar 2022 16:16:13 +0100 Subject: [PATCH 2/2] graph: Assign new user the default user role Similar to what the accounts service is doing, all new users get the User role assigned now. Otherwise creating the user's personal space upon login is not working. --- graph/pkg/service/v0/graph.go | 2 ++ graph/pkg/service/v0/service.go | 10 ++++++---- graph/pkg/service/v0/users.go | 15 +++++++++++++++ 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/graph/pkg/service/v0/graph.go b/graph/pkg/service/v0/graph.go index b634384e33..2dd618d09a 100644 --- a/graph/pkg/service/v0/graph.go +++ b/graph/pkg/service/v0/graph.go @@ -11,6 +11,7 @@ import ( "github.com/owncloud/ocis/graph/pkg/config" "github.com/owncloud/ocis/graph/pkg/identity" "github.com/owncloud/ocis/ocis-pkg/log" + settingssvc "github.com/owncloud/ocis/protogen/gen/ocis/services/settings/v0" "google.golang.org/grpc" ) @@ -66,6 +67,7 @@ type Graph struct { identityBackend identity.Backend gatewayClient GatewayClient httpClient HTTPClient + roleService settingssvc.RoleService spacePropertiesCache *ttlcache.Cache } diff --git a/graph/pkg/service/v0/service.go b/graph/pkg/service/v0/service.go index a2bf0567b8..c77959d963 100644 --- a/graph/pkg/service/v0/service.go +++ b/graph/pkg/service/v0/service.go @@ -116,17 +116,19 @@ func NewService(opts ...Option) Service { svc.httpClient = options.HTTPClient } - roleService := options.RoleService - if roleService == nil { - roleService = settingssvc.NewRoleService("com.owncloud.api.settings", grpc.DefaultClient) + if options.RoleService == nil { + svc.roleService = settingssvc.NewRoleService("com.owncloud.api.settings", grpc.DefaultClient) + } else { + svc.roleService = options.RoleService } + roleManager := options.RoleManager if roleManager == nil { m := roles.NewManager( roles.CacheSize(1024), roles.CacheTTL(time.Hour), roles.Logger(options.Logger), - roles.RoleService(roleService), + roles.RoleService(svc.roleService), ) roleManager = &m } diff --git a/graph/pkg/service/v0/users.go b/graph/pkg/service/v0/users.go index fcce5dc8f2..ebdba6bb8a 100644 --- a/graph/pkg/service/v0/users.go +++ b/graph/pkg/service/v0/users.go @@ -14,6 +14,8 @@ import ( libregraph "github.com/owncloud/libre-graph-api-go" "github.com/owncloud/ocis/graph/pkg/identity" "github.com/owncloud/ocis/graph/pkg/service/v0/errorcode" + settings "github.com/owncloud/ocis/protogen/gen/ocis/services/settings/v0" + settingssvc "github.com/owncloud/ocis/settings/pkg/service/v0" ) // GetMe implements the Service interface. @@ -86,6 +88,19 @@ func (g Graph) PostUser(w http.ResponseWriter, r *http.Request) { return } + // All users get the user role by default currently. + // to all new users for now, as create Account request does not have any role field + if g.roleService == nil { + errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, "could not assign role to account: roleService not configured") + return + } + if _, err = g.roleService.AssignRoleToUser(r.Context(), &settings.AssignRoleToUserRequest{ + AccountUuid: *u.Id, + RoleId: settingssvc.BundleUUIDRoleUser, + }); err != nil { + errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, fmt.Sprintf("could not assign role to account %s", err.Error())) + return + } render.Status(r, http.StatusOK) render.JSON(w, r, u) }