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

server: send non-whitelisted template when email is not in whitelist #13

Merged
merged 1 commit into from
Jan 30, 2020

Conversation

lukateras
Copy link
Contributor

Resolves #12.

Template text is loosely based on https://hackmd.io/_D_JAX7qS9a14GVbE-3vtA. Template can be changed at https://account.postmarkapp.com/servers/5197461/templates/16097548/edit.


if (!await isWhitelisted(payload.email)) { return respond(401) }
if (!await isWhitelisted(email)) {
return sendEmail(email, 'not-whitelisted', {})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return sendEmail(email, 'not-whitelisted', {})
sendEmail(email, 'not-whitelisted', {})
return respond(401)

Copy link
Contributor

Choose a reason for hiding this comment

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

sendEmail() returns a promise of fetch to postmarkapp. You probably want to return 401 (obviously wrapped in Response) no matter what is the result of sendEmail().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, returning sendEmail promise is the right call here. Downstream is in no position to consume 401 given our current flow, as a result it gets ignored anyway. Returning Postmark response in lieu of 401 means that Holo Auth client can make sure that email gets delivered and log its ID, regardless of whether that email is challenge or not-whitelisted.

Copy link
Contributor

Choose a reason for hiding this comment

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

So from what I understand Client is not expecting any logic from the challenge call, right? (haven't checked myself)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So from what I understand Client is not expecting any logic from the challenge call, right? (haven't checked myself)

Correct.

Copy link
Contributor Author

@lukateras lukateras Jan 30, 2020

Choose a reason for hiding this comment

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

Essentially, with this change, POST /v1/challenge response will always be a proxied Postmark response, and that's more useful for us than 401 because it has Postmark message ID that, if logged, can be used for cross-reference by support, and for retries if Postmark service was down.

Copy link
Contributor

@peeech peeech Jan 30, 2020

Choose a reason for hiding this comment

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

means that Holo Auth client can make sure that email gets delivered and log its ID,

ok, now I do not see any logging in Holo Auth Client. I understand you are leaving it open for the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

@lukateras lukateras merged commit 7579624 into develop Jan 30, 2020
@lukateras lukateras deleted the 2019-01-30-not-whitelisted branch January 30, 2020 10:17
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.

Implement “Invalid Email” error handling
2 participants