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

Only require token when unsubscribing #5019

Merged
merged 5 commits into from
Sep 4, 2024
Merged

Conversation

Vinnl
Copy link
Collaborator

@Vinnl Vinnl commented Sep 4, 2024

Since the user is not logged in, there is no extra information given by passing in the email address, but it does add PII to our request logs. Hence, just accepting the unsubscribe token might be enough.

Merges into #4988. /cc @codemist

Since the user is not logged in, there is no extra information
given by passing in the email address, but it does add PII to our
request logs. Hence, just accepting the unsubscribe token might be
enough.
@Vinnl Vinnl added the Review: XS Code review time: up to 30min label Sep 4, 2024
@Vinnl Vinnl requested a review from mansaj September 4, 2024 12:51
Copy link

github-actions bot commented Sep 4, 2024

async function getEmailPreferenceForPrimaryEmail(email: string) {
logger.info("get_email_preference_for_primary_email", {
email,
async function getEmailPreferenceForUnsubscribeToken(unsubscribeToken: string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we are just matching tokens, it doesn't have to use join anymore

getEmailPreferenceForPrimaryEmail,
unsubscribeMonthlyMonitorReportForEmail,
getEmailPreferenceForUnsubscribeToken,
unsubscribeMonthlyMonitorReportForUnsubscribeToken,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Added unique index to the unsub token in the first PR. The index is not necessary anymore

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we add comments to each of these funcs describing what they're for/what they are returning?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, what did we change getEmailPreferenceForPrimaryEmail into? It was being used previously to determine if the monthly report checkbox in the settings page should be selected for the user.

@mansaj
Copy link
Collaborator

mansaj commented Sep 4, 2024

I'll modify this PR and merge it in

@mansaj mansaj merged commit cf331ac into MNTOR-1800-2 Sep 4, 2024
6 of 7 checks passed
@mansaj mansaj deleted the MNTOR-1800-token-only branch September 4, 2024 20:51
Copy link

github-actions bot commented Sep 4, 2024

Cleanup completed - database 'blurts-server-pr-5019' destroyed, cloud run service 'blurts-server-pr-5019' destroyed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: XS Code review time: up to 30min
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants