Skip to content
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

1040 prototype email gateway #1048

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

zaychenko-sergei
Copy link
Contributor

@zaychenko-sergei zaychenko-sergei commented Jan 24, 2025

Description

Closes: #1040

Integration of email gateway (Postmark):

  • Defined EmailSender crate and prototyped Postmark-based implementation
  • CLI config support for gateway settings (API key, sender address & name)
  • Applied askoma templating engine and defined base HTML-rich template for all emails
  • Emails are fire-and-forget / best effort
  • First email implemented: account registration

Added GQL suport to query and update email on the currently logged account.

Emails are mandatory for Kamu accounts now:

  • predefined users need to specify an email in config
  • predefined users are auto-synced at startup in case they existed before
  • GitHub users are queried for primary verified email, even if it is not public
  • migration code for the database existing users

Checklist before requesting a review

  • Unit and integration tests added
  • Compatibility:
    • Network APIs: ✅
    • Workspace layout and metadata: ✅
    • Configuration: ✅
    • Container images: ✅
  • Observability:
    • Tracing / metrics: ✅
    • Alerts: ✅
  • Documentation:
  • Downstream effects:

@zaychenko-sergei zaychenko-sergei linked an issue Jan 24, 2025 that may be closed by this pull request
@zaychenko-sergei zaychenko-sergei marked this pull request as ready for review January 24, 2025 20:08

let account_repo = from_catalog_n!(ctx, dyn AccountRepository);
match account_repo
.update_account_email(&self.account.id, new_email.clone())
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this go through a use case and do auth before changing email? Currently it looks like this will allow anyone (including anonymous users) to change any other account's email.

odf = { workspace = true }
time-source = { workspace = true }

askama = { version = "0.12" }
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I expected to see this dependency in an infra layer, not in a domain crate

@@ -23,12 +23,15 @@ doctest = false

[dependencies]
database-common = { workspace = true }
email-gateway = { workspace = true }
Copy link
Member

Choose a reason for hiding this comment

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

This seems like an odd coupling. I expected that in account domain layer we only emit messages about account life cycle and react to them elsewhere.

The fact that these messages trigger an email notification is IMO neither a account domain concern, nor even an infra-level account service concern. Sending emails notifs is more like Kamu Node-level business logic concern.

I would suggest moving the templating and email sending into a crate in kamu-node. Seems like one of those things that we'll never need on CLI level.

let rendered_registration_body = registration_email.render().unwrap();

self.email_sender
.send_email(
Copy link
Member

Choose a reason for hiding this comment

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

Yea, as per my other comment this looks completely out of place in account domain crate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prototype email gateway
2 participants