Problem/Motivation
SubscriptionManagerService::shouldRedirectCurrentUserOnLogin() decides whether a just-logged-in user without a local subscription should be redirected to the subscribe page. When the redirect setting is on, it instantiates only the default_connector plugin and delegates to that plugin's shouldRedirectCurrentUserOnLogin().
On a multi-connector site this lets one connector's answer speak for all of them. A connector-specific suppression rule (for example "this user has a pending checkout session, do not redirect") on a non-default connector is ignored, and the redirect decision is wrong whenever the default connector is not the one the user is mid-flow with.
Steps to reproduce
- Install connectors A (default) and B, with the
redirectsetting enabled. - Arrange for connector B's
shouldRedirectCurrentUserOnLogin()to returnFALSEfor a user (e.g. an in-progress checkout on B) while A returnsTRUE. - Log in as that user: the user is redirected, because only A was asked.
Proposed resolution
Poll all installed connectors and combine their answers. The natural combination is: redirect only if every connector agrees the user should be redirected (any connector can veto), since a veto means "this user is already engaged with me". Each createInstance() call should be guarded per-connector so a broken plugin cannot break login for the site.
Remaining tasks
Patch plus a kernel test with two test connectors returning opposing answers; agree on veto semantics in the issue before writing the test.
User interface changes
None.
API changes
None to the connector interface; only the service's aggregation behavior changes.
Data model changes
None.
Issue fork subscription_manager-3616773
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
colanComment #4
colanMR opened. Ratifying the aggregation semantics the summary proposed, plus two edge decisions the summary left open:
A side effect worth noting: this retires the last unguarded
createInstance()on rawdefault_connectorconfig — the login path was the one spot the #3616932: Subscribe routes fatal on fresh installs: default_connector is absent from config/install and passed unguarded to createInstance() fresh-install guard could not reach from the controller, and polling by definitions eliminates the config read entirely.Kernel tests (
LoginRedirectPollingTest): unanimous agreement redirects; a non-default connector's veto wins (RED before the fix, which asked only the default); a broken connector abstains rather than vetoing. The test connectors gained state-controlled redirect votes to drive the opposing-answers scenario the summary asked for.Change record drafted for the aggregation change (multi-connector sites now require unanimity where previously only the default connector's answer counted); single-connector sites see no behavior change.
Comment #6
colan