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

  1. Install connectors A (default) and B, with the redirect setting enabled.
  2. Arrange for connector B's shouldRedirectCurrentUserOnLogin() to return FALSE for a user (e.g. an in-progress checkout on B) while A returns TRUE.
  3. 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.

Command icon 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

colan created an issue. See original summary.

colan’s picture

colan’s picture

Status: Active » Needs review

MR opened. Ratifying the aggregation semantics the summary proposed, plus two edge decisions the summary left open:

  • Any veto wins (redirect only if every connector agrees), as proposed — a FALSE means "this user is already engaged with me", and that knowledge can live on any connector.
  • Failures are abstentions, not vetoes: each connector is guarded per the summary, its failure logged at WARNING, and the remaining connectors decide. Treating a crash as a veto would let one broken plugin permanently suppress the redirect site-wide, which is a different bug wearing a safety blanket.
  • No responding connectors → no redirect: the vacuous all-agree must not fire when there is nothing to subscribe to.

A side effect worth noting: this retires the last unguarded createInstance() on raw default_connector config — 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.

  • colan committed 42fbec41 on 1.0.x
    Issue #3616773: Poll every connector for the post-login subscribe...
colan’s picture

Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.