fix(notifications): skip disabled email recipients - #3518
Open
michaelstingl wants to merge 4 commits into
Open
michaelstingl wants to merge 4 commits into
michaelstingl wants to merge 4 commits into
Conversation
An empty mailbox can satisfy an immediate count assertion before an asynchronous notification arrives, hiding unwanted email delivery. Add a bounded observation step using the existing Inbucket helper. Check throughout the observation period and fail when the count differs.
Email notifications can reach disabled users because GetUser does not apply the configured LDAP filters for disabled users. Grouped emails also use the recipient address stored when the events were queued. Look up the recipient again through GetUserByClaim before delivery. This lookup applies the LDAP filters and uses the existing lookup cache. Recheck the global notification setting and use the returned email address. Skip unavailable recipients and log lookup failures without stopping delivery to other recipients. ScienceMesh invitations use the recipient email address from the event without looking up the recipient. Test the internal notification handlers and daily and weekly grouped emails with active users, unavailable recipients, and lookup failures. Related: opencloud-eu#3513
The earlier notification fixes removed these expected failures from the POSIX list but left the two email cases in the decomposed list. Both scenarios pass on the unpatched source base, so the acceptance runner rejects them as unexpected successes. Match the decomposed email expectations to POSIX. Keep the separate in-app notification entry, which the email tests do not cover. Related: opencloud-eu#3257
The existing email scenarios do not check delivery after a user is disabled. An immediate empty-mailbox assertion can also miss email that arrives asynchronously. Extend the existing email feature to cover direct and group shares with cold and expired lookup caches, daily and weekly grouped emails, and file-share removal. Check that active users still receive email and observe disabled recipients' mailbox counts for a bounded period. Use a Graph mention step to check active delivery and the existing API rejection of disabled recipients. The unit tests separately check that events are queued before the recipient is disabled. Related: opencloud-eu#3513
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 74 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
rhafer
reviewed
Sep 15, 2026
| (id.GetTenantId() != "" && u.GetId().GetTenantId() != id.GetTenantId()) { | ||
| return nil, errors.New("recipient lookup returned a missing or mismatched identity") | ||
| } | ||
| if u.GetStatus() == user.UserStatus_USER_STATUS_BLOCKED || strings.TrimSpace(u.GetMail()) == "" || s.disableEmails(ctx, id) { |
Member
There was a problem hiding this comment.
Please note that we're currently not populating the Status field in reva anywhere (. So unless that changes this whole PR is pretty much useless.
Currently in OpenCloud there is no way to figure out on the CS3 API if a user is enabled or disabled.
When adding that enabled/disable feature on the Graph API the only purpose of it was to be able to prevent users from being able to login to OpenCloud. We deliberately accepted that fact that it is still possible to create shares with users that are disabled.
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.
Description
Skip notification emails to users disabled through OpenCloud's user management. This covers share notifications to direct recipients and group members, mentions, and daily or weekly grouped emails, including events stored before disablement.
Related Issue
Related to #3513
Motivation and Context
The check uses
GetUserByClaimbefore sending becauseGetUserdoes not exclude disabled LDAP users.An existing LDAP lookup cache entry can delay suppression until it expires (10 seconds by default). Disabling a user only in an external identity provider is outside this change.
ScienceMesh invitations use an email address without requiring an OpenCloud user, so they skip the recipient lookup.
Lookup errors skip the recipient. Grouped-email events are still removed before delivery and are not retried after a later lookup or SMTP failure.
How Has This Been Tested?
Unit tests and static checks
Acceptance tests
Local runs with Inbucket compare the same source base without and with the fix:
emailNotification.featurepasses with decomposed storage.Types of changes
Checklist:
🤖 drafted with OpenAI Codex, reviewed before submitting.