diff --git a/changelog/unreleased/fix-segfault.md b/changelog/unreleased/fix-segfault.md new file mode 100644 index 000000000..2ba5e1362 --- /dev/null +++ b/changelog/unreleased/fix-segfault.md @@ -0,0 +1,5 @@ +Bugfix: Prevent segfault when no password is set + +Passwords are stored in a dedicated child struct of an account. We fixed several segfault conditions where the methods would try to unset a password when that child struct was not existing. + +https://github.com/owncloud/ocis-accounts/pull/65 diff --git a/changelog/unreleased/tighten-screws.md b/changelog/unreleased/tighten-screws.md new file mode 100644 index 000000000..00d9185e9 --- /dev/null +++ b/changelog/unreleased/tighten-screws.md @@ -0,0 +1,5 @@ +Change: Tighten screws on usernames and email addresses + +In order to match accounts to the OIDC claims we currently rely on the email address or username to be present. We force both to match the [W3C recommended regex](https://www.w3.org/TR/2016/REC-html51-20161101/sec-forms.html#valid-e-mail-address) with usernames having to start with a character or `_`. This allows the username to be presented and used in ACLs when integrating the os with the glauth LDAP service of ocis. + +https://github.com/owncloud/ocis-accounts/pull/65 diff --git a/pkg/proto/v0/accounts.pb.micro_test.go b/pkg/proto/v0/accounts.pb.micro_test.go index a43056c33..fa9482095 100644 --- a/pkg/proto/v0/accounts.pb.micro_test.go +++ b/pkg/proto/v0/accounts.pb.micro_test.go @@ -389,6 +389,11 @@ func TestCreateExistingUser(t *testing.T) { // All tests fail after running this // https://github.com/owncloud/ocis-accounts/issues/62 func TestCreateAccountInvalidUserName(t *testing.T) { + + resp, err := listAccounts(t) + checkError(t, err) + numAccounts := len(resp.GetAccounts()) + testData := []string{ "", "0", @@ -400,14 +405,16 @@ func TestCreateAccountInvalidUserName(t *testing.T) { _, err := createAccount(t, userName) // Should give error - checkError(t, err) + if err == nil { + t.Fatalf("Expected an Error when creating user '%s' but got nil", userName) + } } - // This throws a error in server - // resp contains no accounts - resp, _ := listAccounts(t) + // resp should have the same number of accounts + resp, err = listAccounts(t) + checkError(t, err) - assert.Equal(t, 0, len(resp.GetAccounts())) + assert.Equal(t, numAccounts, len(resp.GetAccounts())) cleanUp(t) } diff --git a/pkg/service/v0/accounts.go b/pkg/service/v0/accounts.go index e940e72ef..5e36b2ec5 100644 --- a/pkg/service/v0/accounts.go +++ b/pkg/service/v0/accounts.go @@ -233,7 +233,10 @@ func (s Service) ListAccounts(ctx context.Context, in *proto.ListAccountsRequest s.expandMemberOf(a) // remove password before returning - a.PasswordProfile.Password = "" + + if a.PasswordProfile != nil { + a.PasswordProfile.Password = "" + } out.Accounts = append(out.Accounts, a) } @@ -259,7 +262,9 @@ func (s Service) GetAccount(c context.Context, in *proto.GetAccountRequest, out s.expandMemberOf(out) // remove password - out.PasswordProfile.Password = "" + if out.PasswordProfile != nil { + out.PasswordProfile.Password = "" + } return } @@ -273,6 +278,12 @@ func (s Service) CreateAccount(c context.Context, in *proto.CreateAccountRequest if in.Account.Id == "" { in.Account.Id = uuid.Must(uuid.NewV4()).String() } + if !s.isValidUsername(in.Account.PreferredName) { + return merrors.BadRequest(s.id, "preferred_name '%s' must be at least the local part of an email", in.Account.PreferredName) + } + if !s.isValidEmail(in.Account.Mail) { + return merrors.BadRequest(s.id, "mail '%s' must be a valid email", in.Account.Mail) + } if id, err = cleanupID(in.Account.Id); err != nil { return merrors.InternalServerError(s.id, "could not clean up account id: %v", err.Error()) @@ -312,6 +323,12 @@ func (s Service) UpdateAccount(c context.Context, in *proto.UpdateAccountRequest if in.Account.Id == "" { return merrors.BadRequest(s.id, "account id missing") } + if !s.isValidUsername(in.Account.PreferredName) { + return merrors.BadRequest(s.id, "preferred_name '%s' must be at least the local part of an email", in.Account.PreferredName) + } + if !s.isValidEmail(in.Account.Mail) { + return merrors.BadRequest(s.id, "mail '%s' must be a valid email", in.Account.Mail) + } if id, err = cleanupID(in.Account.Id); err != nil { return merrors.InternalServerError(s.id, "could not clean up account id: %v", err.Error()) @@ -342,19 +359,24 @@ func (s Service) UpdateAccount(c context.Context, in *proto.UpdateAccountRequest out.Mail = in.Account.Mail // read only? out.Description = in.Account.Description - if in.Account.PasswordProfile != nil && in.Account.PasswordProfile.Password != "" { - // encrypt password - c := crypt.New(crypt.SHA512) - if out.PasswordProfile.Password, err = c.Generate([]byte(in.Account.PasswordProfile.Password), nil); err != nil { - s.log.Error().Err(err).Str("id", id).Interface("account", loggableAccount(in.Account)).Msg("could not hash password") - return merrors.InternalServerError(s.id, "could not hash password: %v", err.Error()) + if in.Account.PasswordProfile != nil { + if out.PasswordProfile == nil { + out.PasswordProfile = &proto.PasswordProfile{} } + if in.Account.PasswordProfile.Password != "" { + // encrypt password + c := crypt.New(crypt.SHA512) + if out.PasswordProfile.Password, err = c.Generate([]byte(in.Account.PasswordProfile.Password), nil); err != nil { + s.log.Error().Err(err).Str("id", id).Interface("account", loggableAccount(in.Account)).Msg("could not hash password") + return merrors.InternalServerError(s.id, "could not hash password: %v", err.Error()) + } + } + // lastPasswordChangeDateTime calculated, see password + out.PasswordProfile.PasswordPolicies = in.Account.PasswordProfile.PasswordPolicies + out.PasswordProfile.ForceChangePasswordNextSignIn = in.Account.PasswordProfile.ForceChangePasswordNextSignIn + out.PasswordProfile.ForceChangePasswordNextSignInWithMfa = in.Account.PasswordProfile.ForceChangePasswordNextSignInWithMfa out.PasswordProfile.LastPasswordChangeDateTime = tsnow } - // lastPasswordChangeDateTime calculated, see password - out.PasswordProfile.PasswordPolicies = in.Account.PasswordProfile.PasswordPolicies - out.PasswordProfile.ForceChangePasswordNextSignIn = in.Account.PasswordProfile.ForceChangePasswordNextSignIn - out.PasswordProfile.ForceChangePasswordNextSignInWithMfa = in.Account.PasswordProfile.ForceChangePasswordNextSignInWithMfa // memberOf read only // createdDateTime read only @@ -382,7 +404,9 @@ func (s Service) UpdateAccount(c context.Context, in *proto.UpdateAccountRequest } // remove password - out.PasswordProfile.Password = "" + if out.PasswordProfile != nil { + out.PasswordProfile.Password = "" + } return } @@ -425,3 +449,25 @@ func (s Service) DeleteAccount(c context.Context, in *proto.DeleteAccountRequest s.log.Info().Str("id", id).Msg("deleted account") return } + +// We want to allow email addresses as usernames so they show up when using them in ACLs on storages that allow intergration 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 (s Service) 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 (s Service) isValidEmail(e string) bool { + if len(e) < 3 && len(e) > 254 { + return false + } + return emailRegex.MatchString(e) +}