Skip to content

Allow users to change their email #8781

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
bdab4af
allow user to change email and set email in user list to read-only
knollengewaechs Jun 4, 2025
8b4ac8b
send verification email
knollengewaechs Jun 4, 2025
3562d51
enable users to edit their own email address
MichaelBuessemeyer Jun 4, 2025
07fabe0
allow to edit own email addres for non admins
MichaelBuessemeyer Jun 4, 2025
209bf8b
add specific column tracking when the last email change was for more…
MichaelBuessemeyer Jun 4, 2025
9774d5a
Merge branch 'master' into users-can-change-email
knollengewaechs Jun 6, 2025
58bcc29
clean up frontend code
knollengewaechs Jun 6, 2025
74fe1f2
add password verification upon email update
MichaelBuessemeyer Jun 14, 2025
5b866a0
Merge branch 'users-can-change-email' of github.com:scalableminds/web…
MichaelBuessemeyer Jun 14, 2025
04151f4
disallow admins updating emails of other users
MichaelBuessemeyer Jun 18, 2025
8d7ed7b
add entry to MIGRATIONS.unreleased.md
MichaelBuessemeyer Jun 18, 2025
8be7adb
format backend
MichaelBuessemeyer Jun 18, 2025
56098bc
Merge branch 'master' into users-can-change-email
MichaelBuessemeyer Jun 18, 2025
9ec548d
remove logging used for testing
MichaelBuessemeyer Jun 18, 2025
d443e02
Merge branch 'users-can-change-email' of github.com:scalableminds/web…
MichaelBuessemeyer Jun 18, 2025
cc94854
Merge branch 'master' into users-can-change-email
fm3 Jun 19, 2025
2a1166d
unused import
fm3 Jun 19, 2025
9dd940d
Merge branch 'users-can-change-email' of github.com:scalableminds/web…
fm3 Jun 19, 2025
7ff58c0
add missing emailChangeDate column to multi
MichaelBuessemeyer Jun 23, 2025
c45795f
apply pr feedback
MichaelBuessemeyer Jun 23, 2025
dae5355
Merge branch 'master' of github.com:scalableminds/webknossos into use…
MichaelBuessemeyer Jun 23, 2025
34bb221
Merge branch 'users-can-change-email' of github.com:scalableminds/web…
MichaelBuessemeyer Jun 23, 2025
c9731b2
include multiuserid in in logging upon email changed by user
MichaelBuessemeyer Jun 24, 2025
4477287
Update unreleased_changes/8671.md
MichaelBuessemeyer Jun 24, 2025
9811f0a
Merge branch 'master' of github.com:scalableminds/webknossos into HEAD
hotzenklotz Jul 14, 2025
ddc9f57
update migration number
knollengewaechs Jul 15, 2025
30e72ab
update import
knollengewaechs Jul 15, 2025
d0ca9c5
remove unused import
MichaelBuessemeyer Jul 15, 2025
6b14824
sort imports and call function rather than route directly
knollengewaechs Jul 15, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 58 additions & 23 deletions app/controllers/UserController.scala
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
package controllers

import play.silhouette.api.Silhouette
import com.scalableminds.util.accesscontext.{DBAccessContext, GlobalAccessContext}
import com.scalableminds.util.accesscontext.GlobalAccessContext
import com.scalableminds.util.tools.{Fox, FoxImplicits}

import models.annotation.{AnnotationDAO, AnnotationService, AnnotationType}
import models.organization.OrganizationService
import models.team._
Expand All @@ -16,13 +15,18 @@ import com.scalableminds.util.objectid.ObjectId

import javax.inject.Inject
import models.user.Theme.Theme
import com.scalableminds.util.tools.{Failure, Full}
import play.silhouette.api.exceptions.ProviderException
import play.silhouette.api.util.Credentials
import play.silhouette.impl.providers.CredentialsProvider
import security.WkEnv

import scala.concurrent.ExecutionContext

class UserController @Inject()(userService: UserService,
userDAO: UserDAO,
multiUserDAO: MultiUserDAO,
credentialsProvider: CredentialsProvider,
organizationService: OrganizationService,
annotationDAO: AnnotationDAO,
teamMembershipService: TeamMembershipService,
Expand Down Expand Up @@ -175,6 +179,7 @@ class UserController @Inject()(userService: UserService,
((__ \ "firstName").readNullable[String] and
(__ \ "lastName").readNullable[String] and
(__ \ "email").readNullable[String] and
(__ \ "password").readNullable[String] and
(__ \ "isActive").readNullable[Boolean] and
(__ \ "isAdmin").readNullable[Boolean] and
(__ \ "isDatasetManager").readNullable[Boolean] and
Expand All @@ -195,13 +200,45 @@ class UserController @Inject()(userService: UserService,
Fox.successful(())
})

private def checkAdminOnlyUpdates(user: User,
isActive: Boolean,
isAdmin: Boolean,
isDatasetManager: Boolean,
oldEmail: String,
email: String)(issuingUser: User): Boolean =
if (isActive && user.isAdmin == isAdmin && oldEmail == email && isDatasetManager == user.isDatasetManager)
private def checkTeamManagerOnlyUpdates(user: User,
experiences: Map[String, Int],
oldExperiences: Map[String, Int],
teams: List[TeamMembership],
oldTeams: List[TeamMembership])(issuingUser: User): Fox[Boolean] =
if (experiences == oldExperiences && teams == oldTeams)
Fox.successful(true)
else userService.isEditableBy(user, issuingUser)

private def checkPasswordIfEmailChanged(user: User, passwordOpt: Option[String], oldEmail: String, email: String)(
issuingUser: User): Fox[Unit] =
if (oldEmail == email) {
Fox.successful(())
} else if (user._id == issuingUser._id) {
passwordOpt match {
case Some(password) =>
val credentials = Credentials(user._id.id, password)
Fox.fromFutureBox(
credentialsProvider
.authenticate(credentials)
.flatMap { loginInfo =>
userService.retrieve(loginInfo).map {
case Some(_) => Full(())
case None => Failure("error.noUser")
}
}
.recover {
case _: ProviderException =>
Failure("user.email.change.passwordWrong")
})
case None => Fox.failure("user.email.change.passwordWrong")
}
} else {
Fox.failure("notAllowed")
}

private def checkAdminOnlyUpdates(user: User, isActive: Boolean, isAdmin: Boolean, isDatasetManager: Boolean)(
issuingUser: User): Boolean =
if (isActive && user.isAdmin == isAdmin && isDatasetManager == user.isDatasetManager)
true
else issuingUser.isAdminOf(user)

Expand All @@ -224,16 +261,6 @@ class UserController @Inject()(userService: UserService,
} yield ()
} else Fox.successful(())

private def checkSuperUserOnlyUpdates(user: User, oldEmail: String, email: String)(issuingUser: User)(
implicit ctx: DBAccessContext): Fox[Unit] =
if (oldEmail == email) Fox.successful(())
else
for {
count <- userDAO.countIdentitiesForMultiUser(user._multiUser)
issuingMultiUser <- multiUserDAO.findOne(issuingUser._multiUser)
_ <- Fox.fromBool(count <= 1 || issuingMultiUser.isSuperUser) ?~> "user.email.onlySuperUserCanChange"
} yield ()

private def preventZeroAdmins(user: User, isAdmin: Boolean) =
if (user.isAdmin && !isAdmin) {
for {
Expand All @@ -256,6 +283,7 @@ class UserController @Inject()(userService: UserService,
case (firstNameOpt,
lastNameOpt,
emailOpt,
passwordOpt,
isActiveOpt,
isAdminOpt,
isDatasetManagerOpt,
Expand All @@ -264,6 +292,7 @@ class UserController @Inject()(userService: UserService,
lastTaskTypeIdOpt) =>
for {
user <- userDAO.findOne(userId) ?~> "user.notFound" ~> NOT_FOUND
// properties that can be changed by team managers and admins only: experiences, team memberships
oldExperience <- userService.experiencesFor(user._id)
oldAssignedMemberships <- userService.teamMembershipsFor(user._id)
firstName = firstNameOpt.getOrElse(user.firstName)
Expand All @@ -276,13 +305,19 @@ class UserController @Inject()(userService: UserService,
assignedMemberships = assignedMembershipsOpt.getOrElse(oldAssignedMemberships)
experiences = experiencesOpt.getOrElse(oldExperience)
lastTaskTypeId = if (lastTaskTypeIdOpt.isEmpty) user.lastTaskTypeId.map(_.id) else lastTaskTypeIdOpt
_ <- Fox.assertTrue(userService.isEditableBy(user, request.identity)) ?~> "notAllowed" ~> FORBIDDEN
_ <- Fox.fromBool(checkAdminOnlyUpdates(user, isActive, isAdmin, isDatasetManager, oldEmail, email)(
issuingUser)) ?~> "notAllowed" ~> FORBIDDEN
_ <- Fox
.runIf(user._id != issuingUser._id)(Fox.assertTrue(userService.isEditableBy(user, request.identity))) ?~> "notAllowed" ~> FORBIDDEN
_ <- checkTeamManagerOnlyUpdates(user,
experiences,
oldExperience,
assignedMemberships,
oldAssignedMemberships)(issuingUser) ?~> "notAllowed" ~> FORBIDDEN
_ <- Fox
.fromBool(checkAdminOnlyUpdates(user, isActive, isAdmin, isDatasetManager)(issuingUser)) ?~> "notAllowed" ~> FORBIDDEN
_ <- checkPasswordIfEmailChanged(user, passwordOpt, oldEmail, email)(issuingUser)
_ <- Fox.fromBool(checkNoSelfDeactivate(user, isActive)(issuingUser)) ?~> "user.noSelfDeactivate" ~> FORBIDDEN
_ <- checkNoDeactivateWithRemainingTask(user, isActive)
_ <- checkNoActivateBeyondLimit(user, isActive)
_ <- checkSuperUserOnlyUpdates(user, oldEmail, email)(issuingUser)
_ <- preventZeroAdmins(user, isAdmin)
_ <- preventZeroOwners(user, isActive)
teams <- Fox.combined(assignedMemberships.map(t =>
Expand Down
2 changes: 1 addition & 1 deletion app/models/user/EmailVerificationService.scala
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class EmailVerificationService @Inject()(conf: WkConf,
): Fox[Boolean] =
for {
multiUser: MultiUser <- multiUserDAO.findOne(user._multiUser) ?~> "user.notFound"
endOfGracePeriod: Instant = multiUser.created + conf.WebKnossos.User.EmailVerification.gracePeriod
endOfGracePeriod: Instant = multiUser.emailChangeDate + conf.WebKnossos.User.EmailVerification.gracePeriod
overGracePeriod = endOfGracePeriod.isPast
} yield !conf.WebKnossos.User.EmailVerification.required || multiUser.isEmailVerified || !overGracePeriod
}
5 changes: 4 additions & 1 deletion app/models/user/MultiUser.scala
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ case class MultiUser(
selectedTheme: Theme = Theme.auto,
created: Instant = Instant.now,
isEmailVerified: Boolean = false,
emailChangeDate: Instant = Instant.now,
isDeleted: Boolean = false
)

Expand Down Expand Up @@ -57,6 +58,7 @@ class MultiUserDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionContext
theme,
Instant.fromSql(r.created),
r.isemailverified,
Instant.fromSql(r.emailchangedate),
r.isdeleted
)
}
Expand Down Expand Up @@ -91,7 +93,8 @@ class MultiUserDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionContext
_ <- run(q"""UPDATE webknossos.multiusers
SET
email = $email,
isEmailVerified = false
isEmailVerified = false,
emailChangeDate = NOW()
WHERE _id = $multiUserId""".asUpdate)
} yield ()

Expand Down
5 changes: 4 additions & 1 deletion app/models/user/UserService.scala
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,10 @@ class UserService @Inject()(conf: WkConf,
}
for {
oldEmail <- emailFor(user)
_ <- Fox.runIf(oldEmail != email)(multiUserDAO.updateEmail(user._multiUser, email))
_ <- Fox.runIf(oldEmail != email)(for {
_ <- multiUserDAO.updateEmail(user._multiUser, email)
_ = logger.info(s"Email of MultiUser ${user._multiUser} changed from $oldEmail to $email")
} yield ())
_ <- userDAO.updateValues(user._id,
firstName,
lastName,
Expand Down
4 changes: 2 additions & 2 deletions conf/application.conf
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ webKnossos {
inviteExpiry = 14 days
ssoKey = ""
emailVerification {
activated = false
required = false
activated = true
required = true
gracePeriod = 7 days # time period in which users do not need to verify their email address
linkExpiry = 30 days
}
Expand Down
24 changes: 24 additions & 0 deletions conf/evolutions/136-extra-column-for-email-changed.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
START TRANSACTION;

do $$ begin ASSERT (select schemaVersion from webknossos.releaseInformation) = 135, 'Previous schema version mismatch'; end; $$ LANGUAGE plpgsql;

DROP VIEW IF EXISTS webknossos.userInfos;
DROP VIEW IF EXISTS webknossos.multiUsers_;

Comment on lines +5 to +7
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider using CASCADE when dropping views to avoid dependency errors
If other objects depend on userInfos or multiUsers_, the DROP VIEW will fail and abort the whole migration. Adding CASCADE is safer in complex schemas.

-DROP VIEW IF EXISTS webknossos.userInfos;
-DROP VIEW IF EXISTS webknossos.multiUsers_;
+DROP VIEW IF EXISTS webknossos.userInfos CASCADE;
+DROP VIEW IF EXISTS webknossos.multiUsers_ CASCADE;
🤖 Prompt for AI Agents
In conf/evolutions/136-extra-column-for-email-changed.sql around lines 5 to 7,
the DROP VIEW statements for userInfos and multiUsers_ do not use CASCADE, which
can cause failures if other objects depend on these views. Modify the DROP VIEW
statements to include CASCADE (e.g., DROP VIEW IF EXISTS webknossos.userInfos
CASCADE;) to ensure dependent objects are also dropped and avoid migration
aborts.

ALTER TABLE webknossos.multiUsers ADD COLUMN emailChangeDate TIMESTAMPTZ NOT NULL DEFAULT NOW();
UPDATE webknossos.multiUsers SET emailChangeDate = created;

Comment on lines +8 to +10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Heavy rewrite risk – split the NOT NULL DEFAULT change into two steps
ADD COLUMN … NOT NULL DEFAULT NOW() forces a full-table rewrite because NOW() is volatile. On large multiUsers tables this can lock the table for a long time.

A less disruptive pattern:

-ALTER TABLE webknossos.multiUsers ADD COLUMN emailChangeDate TIMESTAMPTZ NOT NULL DEFAULT NOW();
+-- 1. add nullable column without default (no rewrite)
+ALTER TABLE webknossos.multiUsers ADD COLUMN emailChangeDate TIMESTAMPTZ;
+
+-- 2. back-fill existing rows
+UPDATE webknossos.multiUsers SET emailChangeDate = created;
+
+-- 3. make it NOT NULL and set the default for new rows
+ALTER TABLE webknossos.multiUsers
+    ALTER COLUMN emailChangeDate SET NOT NULL,
+    ALTER COLUMN emailChangeDate SET DEFAULT NOW();

This avoids the rewrite while achieving the same outcome.

🤖 Prompt for AI Agents
In conf/evolutions/136-extra-column-for-email-changed.sql around lines 8 to 10,
avoid adding the new column with NOT NULL DEFAULT NOW() in one step because it
causes a full-table rewrite and long locks. Instead, first add the column as
nullable without a default, then update all rows to set the column value to
created, and finally alter the column to set NOT NULL and add the default NOW().
This staged approach prevents heavy table locking on large tables.

CREATE VIEW webknossos.multiUsers_ AS SELECT * FROM webknossos.multiUsers WHERE NOT isDeleted;
CREATE VIEW webknossos.userInfos AS
SELECT
u._id AS _user, m.email, u.firstName, u.lastname, o.name AS organization_name,
u.isDeactivated, u.isDatasetManager, u.isAdmin, m.isSuperUser,
u._organization, o._id AS organization_id, u.created AS user_created,
m.created AS multiuser_created, u._multiUser, m._lastLoggedInIdentity, u.lastActivity, m.isEmailVerified
FROM webknossos.users_ u
JOIN webknossos.organizations_ o ON u._organization = o._id
JOIN webknossos.multiUsers_ m on u._multiUser = m._id;
Comment on lines +11 to +20
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Missing emailChangeDate in userInfos view
The purpose of adding the column is to expose it for downstream logic (e.g. grace-period checks). The recreated userInfos view still omits it, forcing every consumer to query multiUsers directly.

Add the column so callers get it “for free”:

-SELECT
-u._id AS _user, m.email, u.firstName, u.lastname, o.name AS organization_name,
+SELECT
+u._id AS _user, m.email, u.firstName, u.lastname, o.name AS organization_name,
+m.emailChangeDate,

Update all select lists and keep the column order consistent with the forward-migration philosophy that views reflect relevant business data.

🤖 Prompt for AI Agents
In conf/evolutions/136-extra-column-for-email-changed.sql around lines 11 to 20,
the userInfos view definition is missing the emailChangeDate column from the
multiUsers table. To fix this, add the emailChangeDate column to the SELECT list
in the userInfos view, placing it in a logical position consistent with the
existing column order to ensure downstream consumers can access it without
querying multiUsers directly.


UPDATE webknossos.releaseInformation SET schemaVersion = 136;

COMMIT TRANSACTION;
23 changes: 23 additions & 0 deletions conf/evolutions/reversions/136-extra-column-for-email-changed.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
START TRANSACTION;

do $$ begin ASSERT (select schemaVersion from webknossos.releaseInformation) = 136, 'Previous schema version mismatch'; end; $$ LANGUAGE plpgsql;

DROP VIEW IF EXISTS webknossos.userInfos;
DROP VIEW IF EXISTS webknossos.multiUsers_;

ALTER TABLE webknossos.multiUsers DROP COLUMN emailChangeDate;

CREATE VIEW webknossos.multiUsers_ AS SELECT * FROM webknossos.multiUsers WHERE NOT isDeleted;
CREATE VIEW webknossos.userInfos AS
SELECT
u._id AS _user, m.email, u.firstName, u.lastname, o.name AS organization_name,
u.isDeactivated, u.isDatasetManager, u.isAdmin, m.isSuperUser,
u._organization, o._id AS organization_id, u.created AS user_created,
m.created AS multiuser_created, u._multiUser, m._lastLoggedInIdentity, u.lastActivity, m.isEmailVerified
FROM webknossos.users_ u
JOIN webknossos.organizations_ o ON u._organization = o._id
JOIN webknossos.multiUsers_ m on u._multiUser = m._id;

UPDATE webknossos.releaseInformation SET schemaVersion = 135;

COMMIT TRANSACTION;
1 change: 1 addition & 0 deletions conf/messages
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ user.email.verification.keyInvalid=Verification key is invalid.
user.email.verification.keyUsed=Verification key has already been used.
user.email.verification.emailDoesNotMatch=This verification key is associated with a different email address.
user.email.verification.linkExpired=The email verification link is expired.
user.email.change.passwordWrong=The password you entered is incorrect.
user.firstName.invalid=Please check your first name for any special characters
user.lastName.invalid=Please check your last name for any special characters
user.configuration.invalid=Could not parse configuration
Expand Down
Loading