-
Notifications
You must be signed in to change notification settings - Fork 43
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
Consider the environment when generating the invite URLs #3783
Conversation
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
// Package email provides the email utilities for minder | ||
package email | ||
// Package awsses provides the email utilities for minder | ||
package awsses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figure this is vendor agnostic and others could in time add for example a gmail
mailchimp
etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. initially I had a wrapper and this interface -
// Service is the email service interface
type Service interface {
SendEmail(ctx context.Context, to, subject, bodyHTML, bodyText string) error
}
But it felt obsolete since the watermill event message payload now includes everything that is needed for another client to be hooked up.
If we or someone else wants to add another email service provider, they have to implement the Register
method (events.Consumer
interface) and that would be enough.
inviteURL := "" | ||
if cfg.Email.MinderURLBase != "" { | ||
// Create the invite URL | ||
inviteURL = fmt.Sprintf("%s/join/%s", cfg.Email.MinderURLBase, userInvite.Code) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we verify the URL is well-formed anywhere? It might make sense to use url.Parse and then also to join with the suffix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
of course, good idea 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed it 👍 Also added the use of db transaction for both creating and updating an invite so we fail in case the base URL fails parsing.
@@ -2307,6 +2307,8 @@ message Invitation { | |||
// project_display is the display name of the project to which the user | |||
// is invited. | |||
string project_display = 10; | |||
// inviteURL is the URL that can be used to accept the invitation. | |||
string invite_url = 11; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't been following this feature too closely, but why is the invite URL configurable at all? Shouldn't this be something set by the server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think both are true, because the invite URL is generated on the backend side, but it is based on where the FE server is. I decided to pass this from the environment through the server config. Is this what you meant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant to ask is, is the invite URL something the user can affect or is it just given to the user in the reply? If it's just in the reply and the user can't change the invite base URL when they create the invite, then it's OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. No, the user has no control over the invite URL and how it's constructed 👍
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Summary
The following PR:
events.Consumer
interface and that should be enough. The message payload includes everything that is needed - email, subject, html and text bodies.Change Type
Mark the type of change your PR introduces:
Testing
Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.
Review Checklist: