Improve entity access check for Reader Revenue Manager

[nfmohit]

Feature Description

Currently, the Modules\Reader_Revenue_Manager::check_service_entity_access() method only checks if the current user can make a list request to the SwG API, which technically does not dictate if they have access to an actual publication.

This method should be updated to also include checks to see if the current publication exists in the said list response.

This should be similar to Modules\Tag_Manager::check_service_entity_access().

---

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

Acceptance criteria

• It should be considered that a user has access to the Reader Revenue Manager service entity only if their list of publications includes a publication with the connected publication ID.

Implementation Brief

• Update the Modules\Reader_Revenue_Manager::check_service_entity_access() method:
  • The response of the listPublications call should be captured in a new variable, e.g. $publications.
  • Instead of returning true unconditionally after the try/catch block, the method should return true only if a publication exists in $publications with the saved publicationID module setting.
  • See Modules\Tag_Manager::check_service_entity_access() for inspiration.

Test Coverage

• In tests/phpunit/integration/Modules/Reader_Revenue_ManagerTest.php:
  • Extend the test coverage for the service entity access check with different module setting combinations.
  • See Tests\Modules\Tag_ManagerTest for inspiration.

QA Brief

• Using the tester plugin, turn on the rrmModule feature flag.
• Set up Site Kit with the Reader Revenue Manager module - create a new publication in the process.
• Go to Site Kit Settings -> Connected Services -> Reader Revenue Manager and click on "Edit". Verify that the publication select appears.
• Using a different browser, log into the site and connect to Site Kit with a different Google account.
• Go to Site Kit Settings -> Connected Services -> Reader Revenue Manager and click on "Edit". Verify that the publication select appears but is disabled.

Changelog entry

• Improve the check for whether a user has access to the Reader Revenue Manager module, requiring the user to have visibility of the connected publication ID.

[techanvil]

IB ✅

[techanvil]

Hi @nfmohit, sorry to only spot this now, but while reviewing the PR for this issue I've realised that taking this approach would lead to some problems.

Specifically, in the case where the currently selected publication has been deleted, no user would pass the test for module access. Therefore:

• We wouldn't actually know if the user has permission to access the module in order to change the selected publication.
• If the module lost its owner and became recoverable, no user would be able to recover the module due to the check for module access in the recover-modules endpoint.

Maybe I've missed something, but it looks like we should reconsider the approach. Please take a look and see what you think.

[nfmohit]

Thank you for raising these concerns, @techanvil! I spoke with @aaemnnosttv about this as well and we think this would not be a problem.

• We wouldn't actually know if the user has permission to access the module in order to change the selected publication.

We use the hasModuleOwnershipOrAccess selector to determine if the user can change the selected publication. This will always return true for the module owner. Hence, they can change the publication in this case.

• If the module lost its owner and became recoverable, no user would be able to recover the module due to the check for module access in the recover-modules endpoint.

At this stage, RRM will not appear as a recoverable module as it is not shareable. If in the future this module does become shareable, we will have to think about this further. I'll add this to my notes.

Please let me know what you think, thank you!

[techanvil]

Thanks @nfmohit. That's a great point about using hasModuleOwnershipOrAccess() so we determine access via the owner rather than the entity access. And, I managed to completely overlook the sharing aspect with regard to recoverable modules 🤦.

So, the code is totally fine as specced and implemented. Thanks to you and @aaemnnosttv for clarifying, sorry for the false alarm!

[kelvinballoo]

QA Update ⚠️

I've executed the steps and when it comes to login under another Google account under another browser, I can still see the Publication select.

The QAB states:
Verify that the publication select does not appear.

In my case, I get the publication select(dropdown) and there is value, albeit it's disabled.
My expectation was for the dropdown not to appear?

It could be just a case of the QAB needing an update also.

Screenshot and video for reference below:

Publication.appears.actually.mov

[nfmohit]

Hi @kelvinballoo. You are right. With #9151 merged, the QAB was outdated. I have updated it. Please let me know if you have any other questions, thank you!

[kelvinballoo]

QA Update ✅

Thanks for confirming @nfmohit

This is verified good. Moving ticket to Approval.

• Using a different browser, and connecting to Site Kit with a different Google account, it was checked that
  • Under Site Kit Settings -> Connected Services -> Reader Revenue Manager and clicking on "Edit". The publication select appears and is disabled. ✅
• When RRM module is turned off, the settings page doesn't exist. ✅