Skip to content

Add/protect blocked send email#8117

Merged
roccotripaldi merged 13 commits intomasterfrom
add/protect-blocked-send-email
Nov 13, 2017
Merged

Add/protect blocked send email#8117
roccotripaldi merged 13 commits intomasterfrom
add/protect-blocked-send-email

Conversation

@roccotripaldi
Copy link
Copy Markdown
Contributor

@roccotripaldi roccotripaldi commented Nov 4, 2017

Todo:
Jetpack side

  • New UI form to send email to the user
  • add to protect class to verify token that we get from .com and check that it is still valid.
  • Only allow user to login with a specific email. Even though the user uses the right email

.COM side

  • new POST API endpoint to send out the email
  • new GET API endpoint to verify the email in the key and redirect the user to the jetpack side
  • new GET API endpoint that jetpack side uses to check if the api key is still valid
  • new email template
  • new token generation class

Testing instructions / p2 post on it's way...

@roccotripaldi roccotripaldi requested a review from a team as a code owner November 4, 2017 19:33
- adds a better UI on the blocked login page
- allows folks to temporarily unblock themselves with a magic link
@roccotripaldi roccotripaldi force-pushed the add/protect-blocked-send-email branch from dbddbd1 to 7f9582c Compare November 6, 2017 11:46
@jeherve jeherve added [Feature] Protect Also known as Brute Force Attack Protection [Pri] Normal [Status] Needs Review This PR is ready for review. New Feature labels Nov 6, 2017
Comment thread modules/protect.php Outdated
*/
function kill_login() {
$ip = jetpack_protect_get_ip();
$ip = jetpack_protect_get_ip();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the spacing off here?

Comment thread modules/protect.php Outdated
array ( 'response' => 403 )
);
require_once dirname( __FILE__ ) . '/protect/blocked-login-page.php';
$blocked_login_page = Jetpack_Protect_Blocked_Login_Page::instance( $ip );
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Spacing appears off.

Copy link
Copy Markdown
Contributor

@gititon gititon left a comment

Choose a reason for hiding this comment

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

There appear to be indentation issues throughout, but otherwise it looks good. Smeelay!

Comment thread modules/protect/blocked-login-page.php Outdated

public function add_args_to_lostpassword_redirect_url( $url ) {
if ( $this->valid_blocked_user_id ) {
$url = ( empty( $url ) ) ? wp_login_url() : $url;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

paren around empty unnecessary

return false;
}

if ( $this->valid_blocked_user_id ) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe put this case first?

$user = get_user_by( 'email', trim( $email ) );

if ( ! $user ) {
return new WP_Error( 'invalid_user', __( 'Oops, could not find a user with that email address.', 'jetpack' ) );
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this somehow be used by a hacker who is already ip blocked to test for valid email addresses/accounts on the site/domain?

Comment thread modules/protect/blocked-login-page.php Outdated
$code = wp_remote_retrieve_response_code( $response );
$result = json_decode( wp_remote_retrieve_body( $response ) );

if ( 429 === $code ) {
Copy link
Copy Markdown
Contributor

@gititon gititon Nov 10, 2017

Choose a reason for hiding this comment

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

Consider a constant like HTTP_STATUS_TOO_MANY_REQUESTS rather than a "magic number"

Comment thread modules/protect/blocked-login-page.php Outdated


function get_html_blocked_login_message() {
//
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you mean to remove this comment?

Comment thread modules/protect/blocked-login-page.php Outdated
<?php printf(
__( '<p><span style="float:left; display:block; margin-right:10px;">%1$s</span>Your IP (%2$s) has been flagged for potential security violations. <a href="%3$s">Learn More</a></p>', 'jetpack' ),
$icon,
str_replace( 'http://', '', esc_url( 'http://' . $this->ip_address ) ),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not determine this value before ob_start like you do for $icon?

@roccotripaldi roccotripaldi merged commit 1bd1e8e into master Nov 13, 2017
@matticbot matticbot removed the [Status] Needs Review This PR is ready for review. label Nov 13, 2017
@jeherve jeherve deleted the add/protect-blocked-send-email branch November 13, 2017 21:37
@jeherve jeherve added [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Nov 13, 2017
@oskosk oskosk added this to the 5.6 milestone Nov 22, 2017
@jeherve jeherve added [Status] Has Changelog and removed [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Nov 24, 2017
jeherve added a commit that referenced this pull request Nov 24, 2017
oskosk pushed a commit that referenced this pull request Nov 28, 2017
* Changelog 5.6: create base for changelog.

* Update changelog with 5.5.1 info.

* Changelog: add #7930 and #8238

* Changelog: add #8076

* Changelog: add #8100

* Changelog: add #8117

* Changelog: add #8141

* Changelog: add #8143

* Changelog: add #8147

* Changelog: add #8149

* Changelog: add #8153

* Changelog: add #8173

* Changelog: add #8184

* Changelog: add #8196

* Changelog: add #8199

* Changelog: add #8093

* Changelog: add #8171

* Changelog: add #8182

* Changelog: add #8202, #8222

* Changelog: add #8228

* Changelog: add #8240

* Changelog: add #8251

* remove AL card change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Protect Also known as Brute Force Attack Protection New Feature [Pri] Normal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants