diff --git a/CHANGELOG.md b/CHANGELOG.md index 757c6d2fd9..25c0b34d5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ For details about compatibility between different releases, see the **Commitment - Providing fixed downlink paths to the `ttn-lw-cli devices downlink {push|replace}` commands using the `-class-b-c.gateways` parameter. The gateways IDs are comma separated, and the antenna index `i` can be provided by suffixing the ID with `:i` (i.e. `my-gateway:0` for antenna index 0). The group index `j` can be provided by suffixing the ID with `:j` (i.e. `my-gateway:0:1` for antenna index 0 and group index 1). The antenna index is mandatory if a group index is to be provided, but optional otherwise. - Gateway registration without gateway EUI not working. - Listing deleted entities is now fixed for both admin and standard users, which previously returned an `account_not_found` error. +- Update to an user's `PrimaryEmailAddress` via a non admin now invalidates the `PrimaryEmailAddressValidatedAt` as it was intended. ### Security diff --git a/pkg/identityserver/user_registry.go b/pkg/identityserver/user_registry.go index c38832066b..2400f2e83e 100644 --- a/pkg/identityserver/user_registry.go +++ b/pkg/identityserver/user_registry.go @@ -213,7 +213,8 @@ func (is *IdentityServer) createUser(ctx context.Context, req *ttnpb.CreateUserR var primaryEmailAddressFound bool for _, contactInfo := range req.User.ContactInfo { - if contactInfo.ContactMethod == ttnpb.ContactMethod_CONTACT_METHOD_EMAIL && contactInfo.Value == req.User.PrimaryEmailAddress { + if contactInfo.ContactMethod == ttnpb.ContactMethod_CONTACT_METHOD_EMAIL && + contactInfo.Value == req.User.PrimaryEmailAddress { primaryEmailAddressFound = true if contactInfo.ValidatedAt != nil { req.User.PrimaryEmailAddressValidatedAt = contactInfo.ValidatedAt @@ -296,8 +297,6 @@ func (is *IdentityServer) createUser(ctx context.Context, req *ttnpb.CreateUserR }) } - // TODO: Send welcome email (https://github.com/TheThingsNetwork/lorawan-stack/issues/72). - if _, err := is.requestContactInfoValidation(ctx, req.User.GetIds().GetEntityIdentifiers()); err != nil { log.FromContext(ctx).WithError(err).Error("Could not send contact info validations") } @@ -322,7 +321,7 @@ func (is *IdentityServer) getUser(ctx context.Context, req *ttnpb.GetUserRequest if ttnpb.HasAnyField(ttnpb.TopLevelFields(req.FieldMask.GetPaths()), "profile_picture") { if is.configFromContext(ctx).ProfilePicture.UseGravatar { if !ttnpb.HasAnyField(req.FieldMask.GetPaths(), "primary_email_address") { - req.FieldMask.Paths = append(req.FieldMask.GetPaths(), "primary_email_address") + req.FieldMask.Paths = ttnpb.AddFields(req.FieldMask.GetPaths(), "primary_email_address") defer func() { if usr != nil { usr.PrimaryEmailAddress = "" @@ -431,8 +430,9 @@ func (is *IdentityServer) updateUser(ctx context.Context, req *ttnpb.UpdateUserR } if !updatedByAdmin { - req.User.PrimaryEmailAddressValidatedAt = nil cleanContactInfo(req.User.ContactInfo) + req.User.PrimaryEmailAddressValidatedAt = nil + req.FieldMask.Paths = ttnpb.AddFields(req.FieldMask.GetPaths(), "primary_email_address_validated_at") } if ttnpb.HasAnyField(req.FieldMask.GetPaths(), "state") { @@ -471,6 +471,9 @@ func (is *IdentityServer) updateUser(ctx context.Context, req *ttnpb.UpdateUserR defer func() { is.setFullProfilePictureURL(ctx, usr) }() } + updatePrimaryEmailAddress := ttnpb.HasAnyField(req.FieldMask.GetPaths(), "primary_email_address") + updateContactInfo := ttnpb.HasAnyField(req.FieldMask.GetPaths(), "contact_info") + err = is.store.Transact(ctx, func(ctx context.Context, st store.Store) (err error) { if ttnpb.HasAnyField(req.FieldMask.GetPaths(), "admin") { if err := isLastAdmin(ctx, st, req.User.Ids); err != nil { @@ -478,41 +481,79 @@ func (is *IdentityServer) updateUser(ctx context.Context, req *ttnpb.UpdateUserR return err } } - updatingContactInfo := ttnpb.HasAnyField(req.FieldMask.GetPaths(), "contact_info") + var contactInfo []*ttnpb.ContactInfo - updatingPrimaryEmailAddress := ttnpb.HasAnyField(req.FieldMask.GetPaths(), "primary_email_address") - if updatingContactInfo || updatingPrimaryEmailAddress { - if updatingContactInfo { - contactInfo, err = st.SetContactInfo(ctx, req.User.GetIds(), req.User.ContactInfo) + + if updateContactInfo { + contactInfo, err = st.SetContactInfo(ctx, req.User.GetIds(), req.User.ContactInfo) + if err != nil { + return err + } + } + + if updatePrimaryEmailAddress { + // If no update was done to contactInfo list then fetch it. + if !updateContactInfo { + contactInfo, err = st.GetContactInfo(ctx, req.User.GetIds()) if err != nil { return err } } - if updatingPrimaryEmailAddress { - if !updatingContactInfo { - contactInfo, err = st.GetContactInfo(ctx, req.User.GetIds()) - if err != nil { - return err - } + oldUser, err := st.GetUser(ctx, req.User.GetIds(), []string{"primary_email_address"}) + if err != nil { + return err + } + oldEmailAddress := oldUser.PrimaryEmailAddress + + foundOldEmailAddress := false + for _, contactInfo := range contactInfo { + if contactInfo.ContactMethod != ttnpb.ContactMethod_CONTACT_METHOD_EMAIL { + continue } - if !ttnpb.HasAnyField(req.FieldMask.GetPaths(), "primary_email_address_validated_at") { - for _, contactInfo := range contactInfo { - if contactInfo.ContactMethod == ttnpb.ContactMethod_CONTACT_METHOD_EMAIL && contactInfo.Value == req.User.PrimaryEmailAddress { - req.User.PrimaryEmailAddressValidatedAt = contactInfo.ValidatedAt - req.FieldMask.Paths = append(req.FieldMask.GetPaths(), "primary_email_address_validated_at") - break - } + if contactInfo.Value == oldEmailAddress { + foundOldEmailAddress = true + + if updatedByAdmin { + req.User.PrimaryEmailAddressValidatedAt = contactInfo.ValidatedAt + req.FieldMask.Paths = ttnpb.AddFields( + req.FieldMask.GetPaths(), "primary_email_address_validated_at", + ) } + + contactInfo.Value = req.User.PrimaryEmailAddress + contactInfo.ValidatedAt = req.User.PrimaryEmailAddressValidatedAt + break } } + + // If the old email was not found on the contact info list it replaces everything with the new email. + if !foundOldEmailAddress { + contactInfo = []*ttnpb.ContactInfo{{ + ContactMethod: ttnpb.ContactMethod_CONTACT_METHOD_EMAIL, + Value: req.User.PrimaryEmailAddress, + ValidatedAt: req.User.PrimaryEmailAddressValidatedAt, + }} + } + contactInfo, err = st.SetContactInfo(ctx, req.User.GetIds(), contactInfo) + if err != nil { + return err + } } + + // In order to avoid double patching the contact info list we exclude it from the update. + // TODO: remove the separate store operations for ContactInfo + // (https://github.com/TheThingsNetwork/lorawan-stack/issues/6515). + req.FieldMask.Paths = ttnpb.ExcludeFields(req.FieldMask.Paths, "contact_info") + usr, err = st.UpdateUser(ctx, req.User, req.FieldMask.GetPaths()) if err != nil { return err } - if updatingContactInfo { + + if updateContactInfo || updatePrimaryEmailAddress { usr.ContactInfo = contactInfo } + return nil }) if err != nil { @@ -533,6 +574,15 @@ func (is *IdentityServer) updateUser(ctx context.Context, req *ttnpb.UpdateUserR }) } + // NOTE: The reason we validate the `updatingPrimaryEmailAddress` is because all changes on the primary email imply + // in a indirect changed to the contact info list. And if not validated the same is reflected on the contact info + // and a new validation should be requested. + if updatePrimaryEmailAddress && usr.PrimaryEmailAddressValidatedAt == nil { + if _, err := is.requestContactInfoValidation(ctx, req.User.GetIds().GetEntityIdentifiers()); err != nil { + log.FromContext(ctx).WithError(err).Error("Could not send contact info validations") + } + } + return usr, nil } diff --git a/pkg/identityserver/user_registry_test.go b/pkg/identityserver/user_registry_test.go index 671117ac9c..0eef1d63f3 100644 --- a/pkg/identityserver/user_registry_test.go +++ b/pkg/identityserver/user_registry_test.go @@ -251,6 +251,16 @@ func TestUsersCRUD(t *testing.T) { usr1 := p.NewUser() usr1.Password = "OldPassword" + usr1.PrimaryEmailAddress = "user-1@email.com" + validatedAtTime := time.Now().Truncate(time.Millisecond) + usr1.PrimaryEmailAddressValidatedAt = ttnpb.ProtoTime(&validatedAtTime) + // NOTE: Remove this when the deprecated field is removed. + // (https://github.com/TheThingsIndustries/lorawan-stack/issues/3830) + usr1.ContactInfo = append(usr1.ContactInfo, &ttnpb.ContactInfo{ // nolint:staticcheck + ContactMethod: ttnpb.ContactMethod_CONTACT_METHOD_EMAIL, + Value: usr1.PrimaryEmailAddress, + ValidatedAt: usr1.PrimaryEmailAddressValidatedAt, + }) key, _ := p.NewAPIKey(usr1.GetEntityIdentifiers(), ttnpb.Right_RIGHT_ALL) creds := rpcCreds(key) @@ -258,137 +268,204 @@ func TestUsersCRUD(t *testing.T) { keyWithoutRights, _ := p.NewAPIKey(usr1.GetEntityIdentifiers()) credsWithoutRights := rpcCreds(keyWithoutRights) - a, ctx := test.New(t) - - testWithIdentityServer(t, func(is *IdentityServer, cc *grpc.ClientConn) { + testWithIdentityServer(t, func(_ *IdentityServer, cc *grpc.ClientConn) { reg := ttnpb.NewUserRegistryClient(cc) - got, err := reg.Get(ctx, &ttnpb.GetUserRequest{ - UserIds: usr1.GetIds(), - FieldMask: ttnpb.FieldMask("name", "admin", "created_at", "updated_at"), - }, creds) - if a.So(err, should.BeNil) && a.So(got, should.NotBeNil) { - a.So(got.Name, should.Equal, usr1.Name) - a.So(got.Admin, should.Equal, usr1.Admin) - a.So(got.CreatedAt, should.Resemble, usr1.CreatedAt) - } + t.Run("Get", func(t *testing.T) { // nolint:paralleltest + a, ctx := test.New(t) - got, err = reg.Get(ctx, &ttnpb.GetUserRequest{ - UserIds: usr1.GetIds(), - FieldMask: ttnpb.FieldMask("ids"), - }, credsWithoutRights) - a.So(err, should.BeNil) + got, err := reg.Get(ctx, &ttnpb.GetUserRequest{ + UserIds: usr1.GetIds(), + FieldMask: ttnpb.FieldMask("name", "admin", "created_at", "updated_at"), + }, creds) + if a.So(err, should.BeNil) && a.So(got, should.NotBeNil) { + a.So(got.Name, should.Equal, usr1.Name) + a.So(got.Admin, should.Equal, usr1.Admin) + a.So(got.CreatedAt, should.Resemble, usr1.CreatedAt) + } - got, err = reg.Get(ctx, &ttnpb.GetUserRequest{ - UserIds: usr1.GetIds(), - FieldMask: ttnpb.FieldMask("attributes"), - }, credsWithoutRights) - if a.So(err, should.NotBeNil) { - a.So(errors.IsPermissionDenied(err), should.BeTrue) - } + got, err = reg.Get(ctx, &ttnpb.GetUserRequest{ + UserIds: usr1.GetIds(), + FieldMask: ttnpb.FieldMask("ids"), + }, credsWithoutRights) + if a.So(err, should.BeNil) { + a.So(got.GetIds(), should.Resemble, usr1.GetIds()) + } - updated, err := reg.Update(ctx, &ttnpb.UpdateUserRequest{ - User: &ttnpb.User{ - Ids: usr1.GetIds(), - Name: "Updated Name", - }, - FieldMask: ttnpb.FieldMask("name"), - }, creds) - if a.So(err, should.BeNil) && a.So(updated, should.NotBeNil) { - a.So(updated.Name, should.Equal, "Updated Name") - } + got, err = reg.Get(ctx, &ttnpb.GetUserRequest{ + UserIds: usr1.GetIds(), + FieldMask: ttnpb.FieldMask("attributes"), + }, credsWithoutRights) + if a.So(err, should.NotBeNil) { + a.So(errors.IsPermissionDenied(err), should.BeTrue) + a.So(got, should.BeNil) + } + }) - updated, err = reg.Update(ctx, &ttnpb.UpdateUserRequest{ - User: &ttnpb.User{ - Ids: usr1.GetIds(), - State: ttnpb.State_STATE_FLAGGED, - StateDescription: "something is wrong", - }, - FieldMask: ttnpb.FieldMask("state", "state_description"), - }, adminUsrCreds) - if a.So(err, should.BeNil) && a.So(updated, should.NotBeNil) { - a.So(updated.State, should.Equal, ttnpb.State_STATE_FLAGGED) - a.So(updated.StateDescription, should.Equal, "something is wrong") - } + t.Run("Update", func(t *testing.T) { // nolint:paralleltest + t.Run("Simple change", func(t *testing.T) { // nolint:paralleltest + a, ctx := test.New(t) + updated, err := reg.Update(ctx, &ttnpb.UpdateUserRequest{ + User: &ttnpb.User{ + Ids: usr1.GetIds(), + Name: "Updated Name", + }, + FieldMask: ttnpb.FieldMask("name"), + }, creds) + if a.So(err, should.BeNil) && a.So(updated, should.NotBeNil) { + a.So(updated.Name, should.Equal, "Updated Name") + } + }) - updated, err = reg.Update(ctx, &ttnpb.UpdateUserRequest{ - User: &ttnpb.User{ - Ids: usr1.GetIds(), - State: ttnpb.State_STATE_APPROVED, - }, - FieldMask: ttnpb.FieldMask("state"), - }, adminUsrCreds) - if a.So(err, should.BeNil) && a.So(updated, should.NotBeNil) { - a.So(updated.State, should.Equal, ttnpb.State_STATE_APPROVED) - } + t.Run("Change state", func(t *testing.T) { // nolint:paralleltest + a, ctx := test.New(t) - got, err = reg.Get(ctx, &ttnpb.GetUserRequest{ - UserIds: usr1.GetIds(), - FieldMask: ttnpb.FieldMask("state", "state_description"), - }, creds) - if a.So(err, should.BeNil) && a.So(got, should.NotBeNil) { - a.So(got.State, should.Equal, ttnpb.State_STATE_APPROVED) - a.So(got.StateDescription, should.Equal, "") - } + updated, err := reg.Update(ctx, &ttnpb.UpdateUserRequest{ + User: &ttnpb.User{ + Ids: usr1.GetIds(), + State: ttnpb.State_STATE_FLAGGED, + StateDescription: "something is wrong", + }, + FieldMask: ttnpb.FieldMask("state", "state_description"), + }, adminUsrCreds) + if a.So(err, should.BeNil) && a.So(updated, should.NotBeNil) { + a.So(updated.State, should.Equal, ttnpb.State_STATE_FLAGGED) + a.So(updated.StateDescription, should.Equal, "something is wrong") + } - passwordUpdateTime := time.Now().Truncate(time.Millisecond) + updated, err = reg.Update(ctx, &ttnpb.UpdateUserRequest{ + User: &ttnpb.User{ + Ids: usr1.GetIds(), + State: ttnpb.State_STATE_APPROVED, + }, + FieldMask: ttnpb.FieldMask("state"), + }, adminUsrCreds) + if a.So(err, should.BeNil) && a.So(updated, should.NotBeNil) { + a.So(updated.State, should.Equal, ttnpb.State_STATE_APPROVED) + } - _, err = reg.UpdatePassword(ctx, &ttnpb.UpdateUserPasswordRequest{ - UserIds: usr1.GetIds(), - Old: "OldPassword", - New: "NewPassword", // Meets minimum length requirement of 10 characters. - }, creds) - a.So(err, should.BeNil) + got, err := reg.Get(ctx, &ttnpb.GetUserRequest{ + UserIds: usr1.GetIds(), + FieldMask: ttnpb.FieldMask("state", "state_description"), + }, creds) + if a.So(err, should.BeNil) && a.So(got, should.NotBeNil) { + a.So(got.State, should.Equal, ttnpb.State_STATE_APPROVED) + a.So(got.StateDescription, should.Equal, "") + } + }) - afterUpdate, err := reg.Get(ctx, &ttnpb.GetUserRequest{ - UserIds: usr1.GetIds(), - FieldMask: ttnpb.FieldMask("password_updated_at"), - }, creds) - if a.So(err, should.BeNil) && a.So(afterUpdate, should.NotBeNil) { - a.So(afterUpdate.PasswordUpdatedAt, should.NotBeNil) - a.So(*ttnpb.StdTime(afterUpdate.PasswordUpdatedAt), should.HappenAfter, passwordUpdateTime) - } + t.Run("PrimaryEmailAddress", func(t *testing.T) { // nolint:paralleltest + t.Run("admin update", func(t *testing.T) { // nolint:paralleltest + a, ctx := test.New(t) + got, err := reg.Update(ctx, &ttnpb.UpdateUserRequest{ + User: &ttnpb.User{ + Ids: usr1.GetIds(), + PrimaryEmailAddress: "new-user-email@email.com", + }, + FieldMask: ttnpb.FieldMask("primary_email_address"), + }, adminUsrCreds) + if a.So(err, should.BeNil) { + a.So(got.PrimaryEmailAddress, should.Equal, "new-user-email@email.com") + a.So(got.PrimaryEmailAddressValidatedAt, should.NotBeNil) + a.So(got.ContactInfo, should.HaveLength, 1) // nolint:staticcheck + a.So(got.ContactInfo[0].Value, should.Equal, "new-user-email@email.com") // nolint:staticcheck + a.So(got.ContactInfo[0].ValidatedAt, should.NotBeNil) // nolint:staticcheck + } + }) - _, err = reg.Delete(ctx, usr1.GetIds(), creds) - a.So(err, should.BeNil) + t.Run("non admin update", func(t *testing.T) { // nolint:paralleltest + a, ctx := test.New(t) + got, err := reg.Update(ctx, &ttnpb.UpdateUserRequest{ + User: &ttnpb.User{ + Ids: usr1.GetIds(), + PrimaryEmailAddress: "second-new-user-email@email.com", + }, + FieldMask: ttnpb.FieldMask("primary_email_address"), + }, creds) + if a.So(err, should.BeNil) { + a.So(got.PrimaryEmailAddress, should.Equal, "second-new-user-email@email.com") + a.So(got.PrimaryEmailAddressValidatedAt, should.BeNil) + a.So(got.ContactInfo, should.HaveLength, 1) // nolint:staticcheck,lll + a.So(got.ContactInfo[0].Value, should.Equal, "second-new-user-email@email.com") // nolint:staticcheck,lll + a.So(got.ContactInfo[0].ValidatedAt, should.BeNil) // nolint:staticcheck,lll + } + }) + }) + }) - _, err = reg.Get(ctx, &ttnpb.GetUserRequest{ - UserIds: usr1.GetIds(), - FieldMask: ttnpb.FieldMask("name"), - }, creds) - if a.So(err, should.NotBeNil) { - // NOTE: For other entities, this would be a NotFound, but in this case - // the user's credentials become invalid when the user is deleted. - a.So(errors.IsUnauthenticated(err), should.BeTrue) - } + t.Run("Update Password", func(t *testing.T) { // nolint:paralleltest + a, ctx := test.New(t) + passwordUpdateTime := time.Now().Truncate(time.Millisecond) - _, err = reg.Get(ctx, &ttnpb.GetUserRequest{ - UserIds: usr1.GetIds(), - FieldMask: ttnpb.FieldMask("name"), - }, adminUsrCreds) - if a.So(err, should.NotBeNil) { - a.So(errors.IsNotFound(err), should.BeTrue) - } + _, err := reg.UpdatePassword(ctx, &ttnpb.UpdateUserPasswordRequest{ + UserIds: usr1.GetIds(), + Old: "OldPassword", + New: "NewPassword", // Meets minimum length requirement of 10 characters. + }, creds) + a.So(err, should.BeNil) + + afterUpdate, err := reg.Get(ctx, &ttnpb.GetUserRequest{ + UserIds: usr1.GetIds(), + FieldMask: ttnpb.FieldMask("password_updated_at"), + }, creds) + if a.So(err, should.BeNil) && a.So(afterUpdate, should.NotBeNil) { + a.So(afterUpdate.PasswordUpdatedAt, should.NotBeNil) + a.So(*ttnpb.StdTime(afterUpdate.PasswordUpdatedAt), should.HappenAfter, passwordUpdateTime) + } + }) - _, err = reg.Purge(ctx, usr1.GetIds(), creds) - if a.So(err, should.NotBeNil) { - a.So(errors.IsPermissionDenied(err), should.BeTrue) - } + t.Run("Delete", func(t *testing.T) { // nolint:paralleltest + a, ctx := test.New(t) - _, err = reg.Purge(ctx, usr1.GetIds(), adminUsrCreds) - a.So(err, should.BeNil) + _, err := reg.Delete(ctx, usr1.GetIds(), creds) + a.So(err, should.BeNil) - // Admin restrictions, cannot remove the only admin in tenant store. - _, err = reg.Delete(ctx, adminUsr.GetIds(), adminUsrCreds) - a.So(errors.IsFailedPrecondition(err), should.BeTrue) - - _, err = reg.Update(ctx, &ttnpb.UpdateUserRequest{ - User: &ttnpb.User{ - Ids: adminUsr.GetIds(), - Admin: false, - }, - FieldMask: ttnpb.FieldMask("admin"), - }, adminUsrCreds) - a.So(errors.IsFailedPrecondition(err), should.BeTrue) + _, err = reg.Get(ctx, &ttnpb.GetUserRequest{ + UserIds: usr1.GetIds(), + FieldMask: ttnpb.FieldMask("name"), + }, creds) + if a.So(err, should.NotBeNil) { + // NOTE: For other entities, this would be a NotFound, but in this case + // the user's credentials become invalid when the user is deleted. + a.So(errors.IsUnauthenticated(err), should.BeTrue) + } + + _, err = reg.Get(ctx, &ttnpb.GetUserRequest{ + UserIds: usr1.GetIds(), + FieldMask: ttnpb.FieldMask("name"), + }, adminUsrCreds) + if a.So(err, should.NotBeNil) { + a.So(errors.IsNotFound(err), should.BeTrue) + } + }) + + t.Run("Purge", func(t *testing.T) { // nolint:paralleltest + a, ctx := test.New(t) + + _, err := reg.Purge(ctx, usr1.GetIds(), creds) + if a.So(err, should.NotBeNil) { + a.So(errors.IsPermissionDenied(err), should.BeTrue) + } + + _, err = reg.Purge(ctx, usr1.GetIds(), adminUsrCreds) + a.So(err, should.BeNil) + }) + + t.Run("Last Admin Cases", func(t *testing.T) { // nolint:paralleltest + a, ctx := test.New(t) + + // Admin restrictions, cannot remove the only admin in tenant store. + _, err := reg.Delete(ctx, adminUsr.GetIds(), adminUsrCreds) + a.So(errors.IsFailedPrecondition(err), should.BeTrue) + + _, err = reg.Update(ctx, &ttnpb.UpdateUserRequest{ + User: &ttnpb.User{ + Ids: adminUsr.GetIds(), + Admin: false, + }, + FieldMask: ttnpb.FieldMask("admin"), + }, adminUsrCreds) + a.So(errors.IsFailedPrecondition(err), should.BeTrue) + }) }, withPrivateTestDatabase(p)) }