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

Add private sites module #10333

Merged
merged 90 commits into from
May 27, 2019
Merged

Add private sites module #10333

merged 90 commits into from
May 27, 2019

Conversation

scruffian
Copy link
Member

@scruffian scruffian commented Oct 16, 2018

This adds a "Private site" module to Jetpack. The code is based on a similar plugin from WordPress.com.

Fixes #7857

Internal reference: p5XAZ9-1Pk-p2

Changes proposed in this Pull Request:

  • A new module to make a site private
  • Changes to the Jetpack interface to activate/deactivate the module

Testing instructions:

Scenario 1

  • Enable the private site module under Settings > Security

Screenshot 2019-04-18 at 9 08 01 PM

  • In wp-admin, the Site Visibility option appears disabled for private sites

49439010-d934eb80-f7b7-11e8-8443-d3f5fa35eeac

  • Visit your site, you should still be able to see it

  • Visit your site in an incognito window, you should see this:

Screenshot 2019-05-21 at 1 25 50 PM

  • Create a new user on your site (with any role). Log in as this new user. You should be able to see the site.

  • Update this user to have no role on this site. Now you should see this:

Screenshot 2019-05-21 at 1 29 44 PM

  • At a glance dashboard widget shows that the site is private

Screenshot 2019-05-21 at 1 32 05 PM

Scenario 2

  • Enable the publicize module under Settings > Sharing and verify that posts are auto-posted to the connected social media account
  • Enable the private site module
  • Create a new post and verify that the new post is not auto-posted.
  • Disable private site module and verify that new posts are now auto-posted.

@scruffian scruffian requested a review from a team as a code owner October 16, 2018 16:40
@jetpackbot
Copy link

jetpackbot commented Oct 16, 2018

Warnings
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 6326e10

@mdawaffe
Copy link
Member

This is a cool and really useful feature. It'll be nice to get it integrated, especially so that WordPress.com will have more complete information about what sites are private (right now, we do some little heuristics checks to get partial information).

For v1, I don't think we should worry about invites/members, feed authentication, or privatizing links. I think the main focus should be getting something simple in place that locks everything down (requiring authentication for normal blog views, REST API, OPML, etc., an appropriate robots.txt response, …). It'd be nice to get a list of what's important first.

I haven't tested this PR, but I'm reasonably certain that much of the more complicated stuff (particularly invites) will not be fully functional as written outside of the WordPress.com infrastructure.

Besides trimming this down to an MVP, another alternative would be to point people at an existing privacy plugin. That might cause us backward-compatibility headaches in the future (though I don't think it would be too bad), and could be an "easy" (ha!) work-around for whatever is blocked by the lack of private sites in Jetpack.

@kraftbj
Copy link
Contributor

kraftbj commented Oct 17, 2018

In terms of something existing, we often suggest https://wordpress.org/plugins/registered-users-only/ to folks by @Viper007Bond. It'll lock down just about everything, but could be smoother with REST requests.

@jeherve jeherve removed this from the 6.7 milestone Oct 26, 2018
@matticbot
Copy link
Contributor

This PR looks like it might contain user tracking functions. We need to make sure that it is GDPR Compliant.

Rules triggering this positive scan:

  • Called "jetpack_activate_module" hook.

cc: @pesieminski

@matticbot
Copy link
Contributor

This PR looks like it might contain user tracking functions. We need to make sure that it is GDPR Compliant.

Rules triggering this positive scan:

  • Called "jetpack_activate_module" hook.

cc: @pesieminski

@scruffian scruffian added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Nov 20, 2018
@matticbot
Copy link
Contributor

This PR looks like it might contain user tracking functions. We need to make sure that it is GDPR Compliant.

Rules triggering this positive scan:

  • Called "jetpack_activate_module" hook.

cc: @pesieminski

@jeherve jeherve added [Status] In Progress and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Nov 20, 2018
@brbrr
Copy link
Contributor

brbrr commented Nov 20, 2018

I noticed modules/shortcodes/js/blocks/vr-block.js included in this PR. Is this block needed here?

@jeherve
Copy link
Member

jeherve commented Nov 20, 2018

I noticed modules/shortcodes/js/blocks/vr-block.js included in this PR. Is this block needed here?

The file was removed from Jetpack a little while ago, so it may be a rebase gone wrong? Either way, the file should indeed be removed from that PR.

@matticbot
Copy link
Contributor

This PR looks like it might contain user tracking functions. We need to make sure that it is GDPR Compliant.

Rules triggering this positive scan:

  • Called "jetpack_activate_module" hook.

cc: @pesieminski

@scruffian
Copy link
Member Author

@brbrr yes, sorry a rebase gone wrong. fixed now.

@matticbot
Copy link
Contributor

This PR looks like it might contain user tracking functions. We need to make sure that it is GDPR Compliant.

Rules triggering this positive scan:

  • Called "jetpack_activate_module" hook.

cc: @pesieminski

@scruffian
Copy link
Member Author

I updated this to conform to the WordPress coding standards.

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

I left a few notes that hopefully will help a bit.

modules/private.php Outdated Show resolved Hide resolved
modules/private.php Outdated Show resolved Hide resolved
modules/private.php Outdated Show resolved Hide resolved
update_option( 'blog_public', 1 );
}

add_action( 'parse_request', 'privatize_blog', 100 );
Copy link
Member

Choose a reason for hiding this comment

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

Is this enough to make sure we block access to RSS feeds, Core's REST API, and WordPress.com's REST API requests for that site?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to block the Core REST API. I guess we don't need to do that for the WordPress.com REST API as this module won't be enabled on WPCOM, since we have a different one there?

Copy link
Member

Choose a reason for hiding this comment

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

As long as the Jetpack site is connected to WordPress.com and the JSON API module is active in Jetpack (it's enabled by default and there is no easy way to deactivate it, the UI is hidden), the site's content will be available via the WordPress.com REST API. Here is an example:

https://public-api.wordpress.com/rest/v1.1/sites/53262711/posts/

Copy link
Member Author

@scruffian scruffian Nov 26, 2018

Choose a reason for hiding this comment

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

It looks like they are already blocked in the WPCOM REST API:

modules/private.php Outdated Show resolved Hide resolved
modules/private/private.php Show resolved Hide resolved
@matticbot
Copy link
Contributor

scruffian, Your synced wpcom patch D27234-code has been updated.

Copy link
Member

@mdawaffe mdawaffe left a comment

Choose a reason for hiding this comment

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

I left a few nitpicks inline.

The "At a Glance" dashboard widget is confusing in one state:

  1. Make sure your site is public.
  2. Go to wp-admin/ → Settings → Reading.
  3. For "Site Visibility", check "Discourage search engines from indexing this site".
  4. Click "Save Changes".
  5. Go to wp-admin/ → Jetpack → Settings → Security.
  6. For "Private site", check "Make your site private"
  7. Go to wp-admin/ (→ Dashboard)
  8. Look at the "At a Glance" widget. See:

    This site is set to private. Make public.
    Search Engines Discouraged

Screen Shot 2019-05-26 at 11 47 06 PM

modules/private/private.php Outdated Show resolved Hide resolved
modules/private/private.php Outdated Show resolved Hide resolved
modules/private/class-jetpack-private.php Outdated Show resolved Hide resolved
modules/private/class-jetpack-private.php Outdated Show resolved Hide resolved
…essage from At a glance dashboard 2. Adds a line break preceeding the 'Search engines are discouraged message'
@niranjan-uma-shankar
Copy link
Contributor

Thanks for the input @mdawaffe .

The "At a Glance" dashboard widget is confusing in one state:

This is now fixed in ee9ac54. For a private site, the 'At a glance' dashboard widget will no longer show the "Search engines are discouraged" message.

@jeherve jeherve dismissed stale reviews from kraftbj and mdawaffe May 27, 2019 14:14

Feedback was addressed

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This tests well for me, looks good. Let's merge this and get Beta testers to take a crack at it!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels May 27, 2019
@jeherve jeherve merged commit 2316b87 into master May 27, 2019
@jeherve jeherve deleted the add/private-sites branch May 27, 2019 14:24
@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 27, 2019
@jeherve jeherve added [Status] Needs Testing We need to add this change to the testing call for this month's release [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 May 27, 2019
jeherve added a commit that referenced this pull request May 27, 2019
jeherve added a commit that referenced this pull request May 27, 2019
* Kick off the changelog

* Add 7.3.1

* Update date and post link

* changelog: add #12219

* changelog: add #12170

* changelog: add #12184

* Changelog: add #12268

* Changelog: add #12081

* Changelog: add #12323

* Changelog: add #12204

* Changelog: add #12269

* Changelog: add #12332

* changelog: add #12339

* changelog: add #12209

* Changelog: add #12319

* Changelog: add #12357

* Changelog: add #12124

* Changelog: add #12373

* Changelog: add #12252

* Changelog: add #12383

* Changelog: add #12372

* changelog: add #12337

* Changelog: add #12290

* Changelog: add #12301

* Changelog: add #12061

* Testing list: add instructions for #12061

* Changelog: add #12393

* Update minimum supported version

See #12287

* Changelog: add #12406

* Testing list: add #12406

* Changelog: add #12277

* Changelog: add #12412

* Changelog: add #11318

* Changelog: add #12328

* Changelog: add #12425

* Changelog: add #12380

* Changelog: add #12428

* Changelog: add #12414

* Changelog: add #12395

* Changelog & Testing list: add #12416, #12417, #12418, and #12348

* changelog: add #12379

* Changelog: add #12341

* changelog: add #12444

* Changelog: add #12434

* Changelog: add #12454

* Changelog: add #12460

* Changelog: add #12463

* Changelog: add #12457

* Changelog / testing list: add #10333

* Changelog: add #12467


Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
@jeherve jeherve restored the add/private-sites branch June 3, 2019 13:59
@scruffian scruffian deleted the add/private-sites branch June 3, 2019 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jetpack sites can't be set to "Private" in the same way WPCOM sites are.