Say which address change actually fires the event - #219
Merged
Conversation
nursoda
added
the
documentation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Member
Author
|
More PRs upcoming on this issue. |
nursoda
force-pushed
the
docs/say-which-address-fires-the-event
branch
from
August 27, 2026 10:57
097fcd6 to
da903b8
Compare
The class docblock claimed that every path clearing the address used for
delivery goes through setSystemEMailAddress() and therefore reaches this
listener. It does not. Deleting the additional address a user had picked as
their notification address calls setPrimaryEMailAddress(''), and
`occ user:setting <uid> settings primary_email` writes or deletes that user
value directly — only `email` and `display_name` are special-cased there. Both
drop settings/primary_email without dispatching anything, so delivery falls back
to the system address, or stops altogether when the account has none.
The account data does emit UserUpdatedEvent on the first path, but before the
primary address is reset, so getEMailAddress() read from it still yields the old
address. That event is therefore no hook for this, which is why the listener
ignores it — a behaviour the test suite already pins.
Behaviour is unchanged: there is nothing to react to, and a listener that never
runs leaves the provider enabled, which is stricter than disabling it. The claim
was the error — it contradicted the threat model's own "A code outlives a
changed notification address" entry. The docblock now limits itself to the
system address, names the two paths that get past it, and points at that entry
so the two do not drift apart again.
Signed-off-by: Olav Seyfarth <olav@seyfarth.de>
nursoda
force-pushed
the
docs/say-which-address-fires-the-event
branch
from
August 27, 2026 10:58
da903b8 to
a31130c
Compare
This was referenced Aug 27, 2026
seyfahni
approved these changes
Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The class docblock of
EMailDeletedclaimed that every path clearing the address used for delivery goes throughIUser::setSystemEMailAddress()and therefore reaches this listener. It does not, and the claim contradicted the app's own threat model.Two paths change the delivery address without dispatching anything usable:
IUser::setPrimaryEMailAddress('')(apps/provisioning_api/lib/Controller/UsersController.php, branchCOLLECTION_EMAIL), which only removes thesettings/primary_emailuser value.occ user:setting <uid> settings primary_emailwrites or deletes that user value directly — onlyemailanddisplay_nameare special-cased incore/Command/User/Setting.php.After either,
getEMailAddress()falls back to the system address, or returnsnullwhen the account has none. The account data does emitUserUpdatedEventon the first path, but before the primary address is reset, sogetEMailAddress()read from it still yields the old address. That event is no hook for this, which is why the listener ignores it — a behaviourEMailDeletedTestalready pins.No behaviour changes. There is nothing to react to, and a listener that never runs leaves the provider enabled. That is stricter than disabling it, never a downgrade to password-only.
The docblock now limits itself to the system address, names the two paths that get past it, and points at the threat model entry "A code outlives a changed notification address", so the two texts do not drift apart again.
Merge after #214. That entry reaches
mainwith it; this branch touches no file #214 touches, so there is no conflict, only an order.Every claim above was verified against a local checkout of nextcloud/server.
🤖 Generated with Claude Code, verified, tweaked and approved by @nursoda.