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

Email: Send pledge confirmation with authentication token. #46

Merged
merged 2 commits into from
Oct 25, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 143 additions & 0 deletions plugins/wporg-5ftf/includes/email.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
<?php

/**
* Helper functions for sending emails, including authentication tokens.
*
* We don't want pledges connected to individual w.org accounts, because that would encourage people to create
* "company" accounts instead of having their contributions show up as real human beings; or because that
* individual will likely eventually leave the company, and "ownership" of the pledge would be orphaned; or
* because we'd have to tie multiple accounts to the pledge (and all the extra time/UX costs associated with that),
* and would still have problems with orphaned ownership, etc.
*
* So instead, we just ask companies to create pledges using a group email (e.g., support@wordcamp.org), and
* we email them time-restricted, once-time-use auth tokens when they want to "log in".
*
* WP "nonces" aren't ideal for this purpose from a security perspective, because they're less secure. They're
* reusable, last up to 24 hours, and have a much smaller search space in brute force attacks. They're only
* intended to prevent CSRF, and should not be used for authentication or authorization.
*
* They also create an inconsistent UX, because a nonce could be valid for 24 hours, or for 1 second, due to their
* stateless nature -- see `wp_nonce_tick()`. That would lead to some situations where a nonce had already expired
* by the time the contributor opened the email and clicked on the link.
*
* So instead, true stateful CSPRN authentication tokens are generated; see `get_authentication_url()` and
* `is_valid_authentication_token()` for details.
*
* For additional background:
* - https://stackoverflow.com/a/35715087/450127 (which is better security advice than ircmarxell's 2010 answer).
*/

namespace WordPressDotOrg\FiveForTheFuture\Email;

defined( 'WPINC' ) || die();

const TOKEN_PREFIX = '5ftf_auth_token_';

// Longer than `get_password_reset_key()` just to be safe. See https://core.trac.wordpress.org/ticket/43546#comment:34
const TOKEN_LENGTH = 32;

/**
* Wrap `wp_mail()` with shared functionality.
*
* @param string $to
* @param string $subject
* @param string $message
*
* @return bool
*/
function send_email( $to, $subject, $message ) {
$headers = array(
'From: WordPress - Five for the Future <donotreply@wordpress.org>',
'Reply-To: support@wordcamp.org',
// todo update address when new one is created
);

return wp_mail( $to, $subject, $message, $headers );
}

/**
* Generate an action URL with a secure, unique authentication token.
*
* @param int $pledge_id
* @param string $action
* @param int $action_page_id The ID of the page that the user will be taken back to, in order to process their
* verification request.
*
* @return string
*/
function get_authentication_url( $pledge_id, $action, $action_page_id ) {
$auth_token = array(
// This will create a CSPRN and is similar to how `get_password_reset_key()` works.
'value' => wp_generate_password( TOKEN_LENGTH, false ),
// todo should encrypt at rest? core doesn't but others do
'expiration' => time() + ( 2 * HOUR_IN_SECONDS ),
);

/*
* Tying the token to a specific pledge is important for security, otherwise companies could get a valid token
* for their pledge, and use it to edit other company's pledges.
*
* Similarly, tying it to specific actions is also important, to protect against CSRF attacks.
*
* This function intentionally requires the caller to pass in a pledge ID and action, so that it can guarantee
* that each token will be unique across pledges and actions.
*/
update_post_meta( $pledge_id, TOKEN_PREFIX . $action, $auth_token );

$auth_url = add_query_arg(
array(
'action' => $action,
'pledge_id' => $pledge_id,
'auth_token' => $auth_token['value'],
),
get_permalink( $action_page_id )
iandunn marked this conversation as resolved.
Show resolved Hide resolved
);

// todo include a "this lnk will expire in 10 hours and after its used once" message too?
// probably, but what's the best way to do that DRYly?
Copy link
Contributor

Choose a reason for hiding this comment

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

function get_link_expiration_message( $hours ) {} ?

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 I'm thinking it'd be nice to maybe have a wrapper for getting the link and getting the expiration message, because theoretically they should always be used together, but it'd be easy for a dev to forget that or not realize it when adding a new message, so it'd be nice to have some kind of structural way to emphasize it.

Not really sure, though.


return $auth_url;
}

/**
* Verify whether or not a given authentication token is valid.
*
* These tokens are more secure than WordPress' imitation nonces, because they can only be used once, and expire
* in a shorter timeframe. Like WP nonces, though, they must be tied to a specific action and post object in order
* to prevent misuse.
*
* @param $pledge_id
* @param $action
* @param $unverified_token
*
* @return bool
*/
function is_valid_authentication_token( $pledge_id, $action, $unverified_token ) {
$verified = false;
$valid_token = get_post_meta( $pledge_id, TOKEN_PREFIX . $action, true );

/*
* Later on we'll compare the value to user input, and the user could input null/false/etc, so let's guarantee
* that the thing we're comparing against is really what we expect it to be.
*/
if ( ! is_array( $valid_token ) || ! array_key_exists( 'value', $valid_token ) || ! array_key_exists( 'expiration', $valid_token ) ) {
return false;
}

if ( ! is_string( $valid_token['value'] ) || TOKEN_LENGTH !== strlen( $valid_token['value'] ) ) {
return false;
}

if ( $valid_token && $valid_token['expiration'] > time() && $unverified_token === $valid_token['value'] ) {
$verified = true;

// Tokens should not be reusable, to increase security.
delete_post_meta( $pledge_id, TOKEN_PREFIX . $action );
// todo when used to manage pledge, token will probably get deleted when viewing, and then they won't be able to save
// fix that when create the manage process, though. for now this works for confirming email address.
// maye pass a `context` param to this function, either 'view' or 'update', and only delete if context is 'update' ?
// make sure view and update functions checks to make sure have valid token, not create though
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe another context would be confirming a linked contributor?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that'd be the same update context as confirming a pledge, because they seem like they have the same requirements (just being used one page load, not for a session/flow across several page loads).

Were you thinking about it a different way, though?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're right, they are the same context. I was getting thrown off by $pledge_id

}

return $verified;
}
52 changes: 49 additions & 3 deletions plugins/wporg-5ftf/includes/pledge-form.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
namespace WordPressDotOrg\FiveForTheFuture\PledgeForm;

use WordPressDotOrg\FiveForTheFuture;
use WordPressDotOrg\FiveForTheFuture\{ Pledge, PledgeMeta, Contributor };
use WordPressDotOrg\FiveForTheFuture\{ Pledge, PledgeMeta, Contributor, Email };
use WP_Error, WP_User;

defined( 'WPINC' ) || die();
Expand All @@ -21,11 +21,12 @@
* @return false|string
*/
function render_form_new() {
$action = filter_input( INPUT_POST, 'action' );
$action = isset( $_GET['action'] ) ? filter_input( INPUT_GET, 'action' ) : filter_input( INPUT_POST, 'action' );
$data = get_form_submission();
$messages = [];
$complete = false;
$directory_url = get_permalink( get_page_by_path( 'pledges' ) );
$view = 'form-pledge-new.php';

if ( 'Submit Pledge' === $action ) {
$processed = process_form_new();
Expand All @@ -35,11 +36,16 @@ function render_form_new() {
} elseif ( 'success' === $processed ) {
$complete = true;
}
} else if ( 'confirm_pledge_email' === $action ) {
$view = 'form-pledge-confirm-email.php';
$pledge_id = filter_input( INPUT_GET, 'pledge_id', FILTER_VALIDATE_INT );
$unverified_token = filter_input( INPUT_GET, 'auth_token', FILTER_SANITIZE_STRING );
$email_confirmed = process_email_confirmation( $pledge_id, $action, $unverified_token );
}

ob_start();
$readonly = false;
require FiveForTheFuture\PATH . 'views/form-pledge-new.php';
require FiveForTheFuture\get_views_path() . $view;

return ob_get_clean();
}
Expand Down Expand Up @@ -111,6 +117,40 @@ function process_form_new() {
return 'success';
}

/**
* Process a request to confirm a company's email address.
*
* @param int $pledge_id
* @param string $action
* @param array $unverified_token
*
* @return bool
*/
function process_email_confirmation( $pledge_id, $action, $unverified_token ) {
$meta_key = PledgeMeta\META_PREFIX . 'pledge-email-confirmed';
$already_confirmed = get_post( $pledge_id )->$meta_key;

if ( $already_confirmed ) {
/*
* If they refresh the page after confirming, they'd otherwise get an error because the token had been
* used, and might be confused and think that the address wasn't confirmed.
*
* This leaks the fact that the address is confirmed, because it will return true even if the token is
* invalid, but there aren't any security/privacy implications of that.
*/
return true;
}

$email_confirmed = Email\is_valid_authentication_token( $pledge_id, $action, $unverified_token );

if ( $email_confirmed ) {
update_post_meta( $pledge_id, $meta_key, true );
wp_update_post( array( 'ID' => $pledge_id, 'post_status' => 'publish' ) );
}

return $email_confirmed;
}

/**
* Render the form(s) for managing existing pledges.
*
Expand Down Expand Up @@ -179,6 +219,12 @@ function process_form_manage() {
__( 'A pledge already exists for this domain.', 'wporg' )
);
}

// todo email any new contributors for confirmation
// notify any removed contributors?
// ask them to update their profiles?
// automatically update contributor profiles?
// anything else?
}

/**
Expand Down
13 changes: 11 additions & 2 deletions plugins/wporg-5ftf/includes/pledge-meta.php
Original file line number Diff line number Diff line change
Expand Up @@ -184,10 +184,19 @@ function render_meta_boxes( $pledge, $box ) {
* @param WP_Post $pledge
*/
function save_pledge( $pledge_id, $pledge ) {
$action = filter_input( INPUT_GET, 'action' );
$get_action = filter_input( INPUT_GET, 'action' );
$post_action = filter_input( INPUT_POST, 'action' );
$ignored_actions = array( 'trash', 'untrash', 'restore' );

if ( $action && in_array( $action, $ignored_actions, true ) ) {
/*
* This is only intended to run when the front end form and wp-admin forms are submitted, not when posts are
* programmatically updated.
*/
if ( 'Submit Pledge' !== $post_action && 'editpost' !== $get_action ) {
return;
}
Comment on lines +191 to +197
Copy link
Contributor

Choose a reason for hiding this comment

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

@iandunn For some reason, this is preventing post meta values from being updated when the values are changed in the metabox on the Edit Pledge screen in wp-admin.

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, I thought it worked there when I tested, but I'll check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, it was. Fixed in ff9767f


if ( $get_action && in_array( $get_action, $ignored_actions, true ) ) {
return;
}

Expand Down
40 changes: 39 additions & 1 deletion plugins/wporg-5ftf/includes/pledge.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

namespace WordPressDotOrg\FiveForTheFuture\Pledge;
use WordPressDotOrg\FiveForTheFuture\Email;

use WordPressDotOrg\FiveForTheFuture;
use WP_Error;
Expand Down Expand Up @@ -127,5 +128,42 @@ function create_new_pledge( $name ) {
'post_status' => 'draft',
);

return wp_insert_post( $args, true );

$pledge_id = wp_insert_post( $args, true );
// The pledge's meta data is saved at this point via `save_pledge_meta()`, which is a `save_post` callback.

if ( ! is_wp_error( $pledge_id ) ) {
send_pledge_verification_email( $pledge_id, get_post()->ID );
}

return $pledge_id;
}

/**
* Email pledge manager to confirm their email address.
*
* @param int $pledge_id The ID of the pledge.
* @param int $action_page_id The ID of the page that the user will be taken back to, in order to process their
* verification request.
*
* @return bool
*/
function send_pledge_verification_email( $pledge_id, $action_page_id ) {
$pledge = get_post( $pledge_id );

$message =
'Thanks for committing to help keep WordPress sustainable! Please confirm this email address ' .
'so that we can accept your pledge:' . "\n\n" .
Email\get_authentication_url( $pledge_id, 'confirm_pledge_email', $action_page_id )
;

// todo include a notice that the link will expire in X hours, so they know what to expect
// need to make that value DRY across all emails with links
// should probably say that on the front end form success message as well, so they know to go check their email now instead of after lunch.

return Email\send_email(
$pledge->{'5ftf_org-pledge-email'},
'Please confirm your email address',
$message
);
}
1 change: 1 addition & 0 deletions plugins/wporg-5ftf/index.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
*/
function load() {
require_once get_includes_path() . 'contributor.php';
require_once get_includes_path() . 'email.php';
require_once get_includes_path() . 'pledge.php';
require_once get_includes_path() . 'pledge-meta.php';
require_once get_includes_path() . 'pledge-form.php';
Expand Down
Loading