IdP change notification emails #4987
Conversation
The path through our system has been outlined in #4926 . Import and adjust the email relevant bits.
This doesn't have to be special to brig. And, these functions can be used for unit testing.
Let's test the real thing.
supersven
force-pushed
the
sventennie/idp-change-notification-emails
branch
from
70ab96d to
6fc7af7
Compare
zebot
added
the
ok-to-test
There was a problem hiding this comment.
Pull request overview
This pull request implements IdP change notification emails for team admins and owners when SAML Identity Providers are created, updated, or deleted via the API. The feature is currently only enabled for multi-ingress setups as a safety precaution.
Changes:
- Added a new
SAMLEmailSubsystemsubsystem to handle IdP change notifications - Implemented email templates in English and German with certificate details
- Modified IdP CRUD operations in Spar to send notifications when multi-ingress is configured
- Refactored template loading code from Brig to wire-subsystems for better reusability
- Added comprehensive test coverage for the email notification functionality
Reviewed changes
Copilot reviewed 64 out of 64 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/wire-subsystems/src/Wire/SAMLEmailSubsystem.hs | New subsystem effect for SAML email notifications |
| libs/wire-subsystems/src/Wire/SAMLEmailSubsystem/Interpreter.hs | Implementation that sends emails to team admins/owners with certificate changes |
| libs/wire-subsystems/src/Wire/EmailSubsystem.hs | Added SendSAMLIdPChanged effect and IdPDetails data type |
| libs/wire-subsystems/src/Wire/EmailSubsystem/Interpreter.hs | Email rendering logic for IdP change notifications |
| services/spar/src/Spar/API.hs | Modified idpCreate, idpDelete, idpUpdate to send notifications |
| libs/wire-subsystems/test/unit/Wire/SAMLEmailSubsystem/InterpreterSpec.hs | Comprehensive tests for email notifications with multiple locales |
| services/spar/test/Test/Spar/Saml/IdPSpec.hs | Tests verifying notification sending behavior |
| libs/wire-subsystems/templates//team/email/ | Email templates for IdP configuration changes |
| libs/wire-subsystems/src/Wire/EmailSubsystem/Template.hs | Moved template loading utilities from Brig for reusability |
| libs/wire-api/src/Wire/API/Routes/Internal/Brig.hs | Added IdpChangedNotification type and SAMLIdPAPI endpoint |
| libs/saml2-web-sso/src/SAML2/WebSSO/* | Added schema instances for SAML types (Issuer, IdPMetadata, IdPId, IdPConfig) |
| libs/extended/src/Data/X509/Extended.hs | Enhanced to provide structured certificate descriptions |
| changelog.d/2-features/send-email-on-idp-change | Documented the new feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SetStatus _userId _status -> undefined | ||
| GetDefaultUserLocale -> undefined | ||
| CheckAdminGetTeamId _userId -> undefined | ||
| SendSAMLIdPChangedEmail notif -> modify (notif :) |
There was a problem hiding this comment.
The notifications list is accumulated in reverse order (using notif : at line 557 prepends to the list). This means when multiple operations occur (like create followed by delete), the notifications will be in reverse chronological order. The test at line 448 expects [IdPDeleted ..., IdPCreated ...], which confirms this behavior. However, this might be counterintuitive since the natural expectation would be chronological order (oldest first). Consider either reversing the list before returning it, or documenting this behavior clearly.
libs/wire-subsystems/src/Wire/SAMLEmailSubsystem/Interpreter.hs
Outdated
Show resolved
Hide resolved
libs/wire-subsystems/test/unit/Wire/SAMLEmailSubsystem/InterpreterSpec.hs
Outdated
Show resolved
Hide resolved
|
@supersven I've opened a new pull request, #4989, to work on those changes. Once the pull request is ready, I'll request review from you. |
supersven
force-pushed
the
sventennie/idp-change-notification-emails
branch
from
9b0666b to
f47d6bc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 65 out of 65 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| IdPCreated nofifUid _idp -> nofifUid | ||
| IdPDeleted nofifUid _idp -> Just nofifUid | ||
| IdPUpdated nofifUid _old _new -> Just nofifUid |
There was a problem hiding this comment.
Typo in variable name: "nofifUid" should be "notifUid" (missing 't')
| IdPCreated nofifUid _idp -> nofifUid | |
| IdPDeleted nofifUid _idp -> Just nofifUid | |
| IdPUpdated nofifUid _old _new -> Just nofifUid | |
| IdPCreated notifUid _idp -> notifUid | |
| IdPDeleted notifUid _idp -> Just notifUid | |
| IdPUpdated notifUid _old _new -> Just notifUid |
| IdPCreated nofifUid _idp -> nofifUid | ||
| IdPDeleted nofifUid _idp -> Just nofifUid | ||
| IdPUpdated nofifUid _old _new -> Just nofifUid |
There was a problem hiding this comment.
Typo in variable name: "nofifUid" should be "notifUid" (missing 't')
| IdPCreated nofifUid _idp -> nofifUid | |
| IdPDeleted nofifUid _idp -> Just nofifUid | |
| IdPUpdated nofifUid _old _new -> Just nofifUid | |
| IdPCreated notifUid _idp -> notifUid | |
| IdPDeleted notifUid _idp -> Just notifUid | |
| IdPUpdated notifUid _old _new -> Just notifUid |
| IdPCreated nofifUid _idp -> nofifUid | ||
| IdPDeleted nofifUid _idp -> Just nofifUid | ||
| IdPUpdated nofifUid _old _new -> Just nofifUid |
There was a problem hiding this comment.
Typo in variable name: "nofifUid" should be "notifUid" (missing 't')
| IdPCreated nofifUid _idp -> nofifUid | |
| IdPDeleted nofifUid _idp -> Just nofifUid | |
| IdPUpdated nofifUid _old _new -> Just nofifUid | |
| IdPCreated notifUid _idp -> notifUid | |
| IdPDeleted notifUid _idp -> Just notifUid | |
| IdPUpdated notifUid _old _new -> Just notifUid |
Ticket: https://wearezeta.atlassian.net/browse/WPB-22124
Checklist
changelog.d