IdP change notification emails #4987
Conversation
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/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.
libs/wire-subsystems/src/Wire/SAMLEmailSubsystem/Interpreter.hs
Outdated
Show resolved
Hide resolved
libs/wire-subsystems/src/Wire/SAMLEmailSubsystem/Interpreter.hs
Outdated
Show resolved
Hide resolved
libs/wire-subsystems/src/Wire/SAMLEmailSubsystem/Interpreter.hs
Outdated
Show resolved
Hide resolved
supersven
force-pushed
the
sventennie/idp-change-notification-emails
branch
from
a3cb027 to
886b626
Compare
If multi-ingress is configured, call a brig endpoint on every IdP change. Brig will then send an email to all team admins and owners.
supersven
force-pushed
the
sventennie/idp-change-notification-emails
branch
2 times, most recently
from
4ec7105 to
6a987d3
Compare
This replaces prior ToJSON/FromJSON and openapi3 ToSchema instances. IdP will later be used in other entities that should get their ToJSON/FromJSON instances via ToSchema. There's a golden test on `master` which ensures we aren't breaking anything with this.
Move team template readers to EmailSubsystem Such that they can be re-used in unit tests.
This ensures we can actually render templates with the data of the brig endpoint.
supersven
force-pushed
the
sventennie/idp-change-notification-emails
branch
from
6a987d3 to
1c38dbd
Compare
Ticket: https://wearezeta.atlassian.net/browse/WPB-22124
Checklist
changelog.d