Feat/better config UX #58

adekbadek · 2024-02-19T19:43:40Z

All Submissions:

Have you followed the Newspack Contributing guideline?
Does your code follow the WordPress' coding standards and VIP Go coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Adds better and less error-prone UX around adding nodes.

See 1205909204837811-as-1206321490407427/f

How to test the changes in this Pull Request:

Start with a new node or unlink an existing node*
Create a new Node in the Hub's Nodes view
Click "Link the site" and observe a new browser window opening with the Node site's admin (or login page)

Click "Save Changes" on the Node site admin panel to link the sites
Click the link from the Hub again and observe a notice saying "This site is already linked to this Hub.":

* wp option delete newspack_node_hub_url newspack_node_secret_key newspack_network_site_role

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully ran tests with your changes locally?

This will sync only latest 20 events, so let's not suggest otherwise

leogermani · 2024-02-20T20:25:57Z

includes/class-initializer.php

@@ -29,8 +29,11 @@ public static function init() {
 			Hub\Newspack_Ads_GAM::init();
 		}

-		if ( Site_Role::is_node() ) {
+		// Allow to access node settings before the site has a role, so it can be set via URL.
+		if ( Site_Role::is_node() || ! Site_Role::get() ) {


With this change the "Node settings" menu is visible even before I define the site's role. Don't think it's ideal;

WDYT to process the incoming link in the "Site Role" settings page instead?

Fixed in faab389. Moving the incoming link processing to "Site Role" would not allow the code to leverage the existing settings-updating code.

leogermani · 2024-02-20T20:26:19Z

includes/class-admin.php

+	/**
+	 * Is updating the Node settings from URL?
+	 */
+	public static function is_updating_from_url() {


Can this live in the class that will actually process the request instead?

leogermani · 2024-02-20T20:30:09Z

includes/node/class-settings.php

+			// Remove the params from URL - settings were just saved.
+			if ( $referrer ) {
+				if ( ! \Newspack_Network\Site_Role::get() ) {
+					update_option( \Newspack_Network\Site_Role::OPTION_NAME, \Newspack_Network\Site_Role::NODE_ROLE );


Nit, but a \Newspack_Network\Site_Role::set_as_node() method would look prettier

leogermani · 2024-02-20T20:44:07Z

includes/hub/class-nodes.php

+		return add_query_arg(
+			[
+				'page'       => \Newspack_Network\Node\Settings::PAGE_SLUG,
+				'secret_key' => $secret_key,


maybe not a huge deal, but it's not best practice to share a secret key like this.

We should create a temporary key, that expires within minutes, that will allow the node to make a request to the Hub to fetch the key

I'm not sure this actually would increase security. The secret key would end up in a request URL anyway, and if someone has access to server logs they'd see the URL param from any request. And I don't think the risk warrants the added complexity.

Not exactly. The secret key would be in the body of a POST response, encrypted by https. no middle man can see it and no risk of this URL be in the browser history, or the user accidentally sharing it.

I don't think we need to be radical and 100% secure and follow all security recommendations, if we were to do that we would probably use a pair of private/public keys for each site (this would increase complexity) or use Application passwords in both ends (which would also increase complexity)... but having the secret key to be exposed like that in the URL I think is going to radical in the opposite direction

Query string is also encrypted by HTTPS, but you're right that the secret key would be persisted in browser history. Still a pretty elaborate attack scenario, but I agree it's not ideal.

(improved security add in #64 by @leogermani)

# [1.3.0-alpha.1](v1.2.0...v1.3.0-alpha.1) (2024-02-23) ### Bug Fixes * memberships sync ([#63](#63)) ([0a54f1d](0a54f1d)) ### Features * add hub bookmarks to nodes ([#56](#56)) ([54700a0](54700a0)) * add option to manually sync users ([#53](#53)) ([3ec1b19](3ec1b19)) * display membership plans from the network ([#59](#59)) ([6fa01fd](6fa01fd)) * enhance network subscriptions and orders search ([#55](#55)) ([bcb0615](bcb0615)) * Node connection using a link ([#58](#58)) ([721f8b2](721f8b2)) * **wp-cli:** add command to backfill events ([#51](#51)) ([13d2498](13d2498)) * **wp-cli:** add process pending webhook requests command ([#67](#67)) ([7dbd8dc](7dbd8dc)) * **wp-cli:** sync all events from the Hub ([#65](#65)) ([dc595ca](dc595ca))

matticbot · 2024-02-23T15:15:28Z

🎉 This PR is included in version 1.3.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

# [1.3.0](v1.2.0...v1.3.0) (2024-02-28) ### Bug Fixes * backfill duplicate handling; woo links in wp-admin-bar ([#71](#71), [#72](#72)) ([bbce13b](bbce13b)) * cli commands ([#73](#73)) ([ff563ac](ff563ac)) * memberships sync ([#63](#63)) ([0a54f1d](0a54f1d)) * missing woocommerce-memberships handling ([76dbdf7](76dbdf7)) ### Features * add hub bookmarks to nodes ([#56](#56)) ([54700a0](54700a0)) * add option to manually sync users ([#53](#53)) ([3ec1b19](3ec1b19)) * display membership plans from the network ([#59](#59)) ([6fa01fd](6fa01fd)) * enhance network subscriptions and orders search ([#55](#55)) ([bcb0615](bcb0615)) * Node connection using a link ([#58](#58)) ([721f8b2](721f8b2)) * **wp-cli:** add command to backfill events ([#51](#51)) ([13d2498](13d2498)) * **wp-cli:** add process pending webhook requests command ([#67](#67)) ([7dbd8dc](7dbd8dc)) * **wp-cli:** sync all events from the Hub ([#65](#65)) ([dc595ca](dc595ca))

matticbot · 2024-02-28T13:09:58Z

🎉 This PR is included in version 1.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

adekbadek added 2 commits February 19, 2024 18:55

fix: better CTA message

e6ee21c

This will sync only latest 20 events, so let's not suggest otherwise

feat: click a link to link node to hub

f43e503

adekbadek added the [Status] Needs Review label Feb 19, 2024

adekbadek requested a review from a team as a code owner February 19, 2024 19:43

leogermani reviewed Feb 20, 2024

View reviewed changes

adekbadek added 3 commits February 21, 2024 12:32

refactor: cleaner

530867f

refactor: method move

364db41

fix: hide some UI if not yet configured

faab389

adekbadek requested a review from leogermani February 21, 2024 12:06

leogermani mentioned this pull request Feb 21, 2024

feat: better security on automatic connection #64

Merged

feat: better security on automatic connection (#64)

80ff68a

leogermani approved these changes Feb 22, 2024

View reviewed changes

adekbadek merged commit 721f8b2 into trunk Feb 22, 2024
3 checks passed

adekbadek deleted the feat/better-config-ux branch February 22, 2024 14:38

matticbot added the released on @alpha label Feb 23, 2024

matticbot added the released label Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/better config UX #58

Feat/better config UX #58

adekbadek commented Feb 19, 2024

leogermani Feb 20, 2024

adekbadek Feb 21, 2024

leogermani Feb 20, 2024

adekbadek Feb 21, 2024

leogermani Feb 20, 2024

adekbadek Feb 21, 2024

leogermani Feb 20, 2024

adekbadek Feb 21, 2024

leogermani Feb 21, 2024

adekbadek Feb 21, 2024

adekbadek Feb 22, 2024

matticbot commented Feb 23, 2024

matticbot commented Feb 28, 2024

Feat/better config UX #58

Feat/better config UX #58

Conversation

adekbadek commented Feb 19, 2024

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matticbot commented Feb 23, 2024

matticbot commented Feb 28, 2024