Do not show TwG module for non-HTTPS site

[felixarntz]

Since the TwG JS library requires the consuming site to use HTTPS, we should (at least for now) completely hide the module on sites that don't use it (e.g. "http://" or "localhost" sites).

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• If the current site (not reference site) does not use "https://", the Thank with Google module should not surface for the site at all.
  • If desirable from an engineering perspective, this could potentially be bypassed for development sites.

Implementation Brief

• In includes/Core/Modules/Modules.php:
  • Locate the addition of Thank_with_Google to the $core_modules property. https://github.com/google/site-kit-wp/blob/f2e51c6156f7693dae51dc15363a313775e565a9/includes/Core/Modules/Modules.php#L191-L193
  • Update the condition so that besides checking if the feature is enabled, the following conditions should also be met:
    • The site is on HTTPS
      • using the core is_ssl function.
      • additionally, check the HTTP_X_FORWARDED_PROTO header if is_ssl returns false to take care of sites behind proxies (example).
    • OR If the site isn't on HTTPS, the build mode is development (using the Build_Mode::get_mode() method).

Test Coverage

• The ModulesTest->test_feature_flag_enabled_modules() test case may need updating as Thank with Google will no longer be restricted using the feature flag.

QA Brief

Changelog entry

[felixarntz]

@eclarke1 @FlicHollis This is a new launch blocker, but should be very small. There is really nothing to test here comprehensively, so this can simply happen anytime before the e2e test. It does not block the bug bash.

[eclarke1]

Thanks @felixarntz as it is not a bug bash blocker, should we set this to a P1 status then? We have other P1s in the GH board to indicate this is required for launch, but not bug bash. Once we have the bug bash, at that point I suggest we then re-purpose the labels to indicate P0 is a launch blocker, and P1 are nice to have for launch.

[felixarntz]

@eclarke1 Sounds good, updated!

[aaemnnosttv]

@felixarntz maybe for non-https sites we should still show the module but it should be disabled to not allow it to be enabled like we do for AdSense when an adblocker is detected?

[eugene-manuilov]

In includes/Core/Util/BC_Functions.php:

• Add a new protected static method named is_ssl. This method should basically be an improved version of the core is_ssl() function so that it supports checking for sites using Cloudflare and other proxies. It can be copied from here.

@nfmohit, the is_ssl function exists from WP 2.6.0, there is no need to add a backward compatibility for it, we can use that function as is.

[nfmohit]

@nfmohit, the is_ssl function exists from WP 2.6.0, there is no need to add a backward compatibility for it, we can use that function as is.

@eugene-manuilov The is_ssl function as is doesn't work for sites behind some proxy/load balancer (e.g. Cloudflare). Which is why I thought improving it with something like this would be a nice to have.

Do you think this isn't necessary, or if this is necessary, shall we place it somewhere other than BC_Functions?

Thank you!

[eugene-manuilov]

@nfmohit, yes, there might be some cases when the is_ssl function may return incorrect results... however I am not sure that we have to add a vendor specific checks to our plugin. What we should probably do is to use the is_ssl function as is in the Modules class and additionally check the HTTP_X_FORWARDED_PROTO header if is_ssl returns false. So, no need to add anything to the BC_Functions class.

[nfmohit]

Thank you @eugene-manuilov! I have updated the IB.

[eugene-manuilov]

IB ✔️

[mohitwp]

QA Update ✅

Verified

• Done set up on site without SSL.
• The Thank with Google module is not showing up in the Settings > Connect More services.
• Also verified for the case if site SSL certificate gets expired and TwG module is already connected in this case also 'Thank with Google' is not showing on connected services page and site require user to resign in again.

https://user-images.githubusercontent.com/94359491/187642479-7940eefa-1165-444e-be2a-bcef1686922b.mp4

Note : Assigning to QA eng. as per QAB.

[nfmohit]

QA:Eng Verified ✅

• Built the plugin in development mode with npm run build:dev.
• The TwG module still shows up in non-https sites. ie. http://sitekit.10uplabs.com/

[felixarntz]

Approval ⚠️

@kuasha420 @nfmohit @eugene-manuilov We'll approve this for now, but using the is_ssl() function is not really the most suitable approach here, since it is more focused on the current request rather than how the site is configured. Since we need HTTPS primarily in the frontend of the site, we should instead rely on something like home_url().

This doesn't need to be fixed in the release though, so I opened #5806 to address this in a follow up. Please have a look.