feat(ics): add HTTP Basic Authentication for subscription feeds

[JohnVillalovos]

Introduce an optional Basic Auth gate on the ICS and Atom subscription endpoints, controlled by the new ics.subscription.basic.auth config key

When enabled, SubscriptionPage::tryBasicAuth() runs before the presenter: on missing or invalid credentials it sets a 401 with WWW-Authenticate so calendar clients can prompt; on success it establishes a real server-side session via IWebAuthentication::LoginForFeed(). LoginForFeed reuses the normal user-session construction but skips the last-login database write, since feed clients poll on every refresh.

[Copilot]

Pull request overview

Adds an optional HTTP Basic Authentication gate to calendar subscription feeds (ICS/Atom) so feed consumers can authenticate per-request (without creating a persistent browser session), enabling “fully authenticated” feed rendering when credentials are provided.

Changes:

• Introduces SubscriptionPage::tryBasicAuth() and a shared subscription-feed request lifecycle (gate → presenter → render).
• Adds feed-scoped session support (GetFeedUserSession()) so presenters can render with authenticated context when Basic Auth succeeds.
• Adds configuration and tests for the new Basic Auth feature.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
tests/Presenters/CalendarSubscriptionPresenterTest.php	Extends the test page double with SetIsNotFound() and feed-session accessors.
tests/Pages/Export/CalendarSubscriptionPageTest.php	New PHPUnit coverage for tryBasicAuth() behavior (gates + basic-auth branches).
tests/fakes/FakeWebAuthentication.php	Adds fake support for LoginForFeed() and validation call tracking.
tests/fakes/FakeAuth.php	Implements new BuildSession() API on the authentication fake.
tests/Application/Authentication/AuthenticationTest.php	Updates PHPDoc type annotation to match the testable auth class.
Presenters/CalendarSubscriptionPresenter.php	Uses feed-scoped session when provided (fallback to server session otherwise).
Pages/Export/SubscriptionPage.php	Adds notFound flag, feed session storage, Basic Auth gate, and a shared PageLoad() template.
Pages/Export/ICalendarSubscriptionPage.php	Extends the page contract with SetIsNotFound() and GetFeedUserSession().
Pages/Export/CalendarSubscriptionPage.php	Converts to renderFeed() implementation (rendering only).
Pages/Export/AtomSubscriptionPage.php	Converts to renderFeed() implementation (rendering only).
lib/Config/ConfigKeys.php	Adds new config key definition for enabling ICS Basic Auth.
lib/Application/Authentication/WebAuthentication.php	Adds/implements LoginForFeed() on IWebAuthentication.
lib/Application/Authentication/IAuthentication.php	Adds BuildSession() to build a session without recording a login event.
lib/Application/Authentication/Authentication.php	Implements BuildSession() and makes GetUserSession() extensible (protected).
config/config.dist.php	Adds ics[basic.auth] default config entry.
.env.example	Adds LB_ICS_BASIC_AUTH env var example.

[JohnVillalovos]

Codex GPT-5.5 Medium review

Code Review: 969783f

Commit: feat(ics): add HTTP Basic Authentication for subscription feeds

Recommendation

Do not merge upstream yet.

The feature direction is reasonable, but the commit has a regression in the core subscription authorization path and a runtime break for common authentication plugins.

Findings

Blocker: Invalid subscription validation now renders a feed instead of returning 404

Pages/Export/SubscriptionPage.php:197 calls $this->presenter->PageLoad() but ignores its boolean return value.

Presenters/CalendarSubscriptionPresenter.php:50 returns false when validation fails, but it does not call SetIsNotFound(). The old CalendarSubscriptionPage and AtomSubscriptionPage implementations checked that return value and set http_response_code(404).

That means a bad icskey, an empty configured subscription key, disabled validation path, or a disallowed resource/schedule/user can fall through to renderFeed() and return a 200 response, usually with an empty feed body.

This should be fixed before merge, either by checking the presenter's return value in SubscriptionPage::PageLoad() or by making the presenter/validator consistently signal not-found through the page.

Blocker: Basic Auth with decorated auth plugins can fatal after successful validation

lib/Application/Authentication/WebAuthentication.php:130 calls BuildSession() on whatever authentication implementation PluginManager returns.

The commit adds BuildSession() to Authentication, but many auth plugins extend Authentication while decorating the real base auth object and do not call Authentication::__construct(). Examples:

• plugins/Authentication/Apache/Apache.php:19
• plugins/Authentication/Ldap/Ldap.php:83

Those plugins inherit Authentication::BuildSession(), which dereferences $this->userRepository in lib/Application/Authentication/Authentication.php:151. Because those decorator plugins never initialize the base repositories, Basic Auth can fail at runtime after successful credential validation.

The fix should either make each decorator plugin override/delegate BuildSession() to its wrapped authentication object, or avoid adding this method to the plugin contract in this form.

Minor: Commit message names the wrong config key

The commit message says the feature is controlled by ics.subscription.basic.auth, but the actual key is ics.basic.auth in lib/Config/ConfigKeys.php:1118.

The message should be amended so reviewers and operators are not pointed at a nonexistent setting.

[JohnVillalovos]

Claude Sonnet 5 Review

Review: 969783f05 — feat(ics): add HTTP Basic Authentication for subscription feeds

Recommendation: do not merge yet. The refactor introduces a control-flow regression that silently defeats the existing subscription-key/subscription-allowed validation on both feed endpoints.

The bug (high confidence, verified against HEAD~1)

Pages/Export/SubscriptionPage.php:186-207, the new template method:

public function PageLoad()
{
    $this->tryBasicAuth();
    if ($this->notFound) {
        return;
    }

    $this->presenter->PageLoad();          // <-- return value discarded
    if ($this->notFound) {
        http_response_code(404);
        return;
    }

    $this->renderFeed();
}

CalendarSubscriptionPresenter::PageLoad(): bool returns false when
CalendarSubscriptionValidator::IsValid() fails — wrong/missing subscription
key, ICS disabled, or GetIsCalendarSubscriptionAllowed() is false for the
target resource/schedule/user
(lib/Application/Schedule/CalendarSubscriptionValidator.php:30-64). Nothing
in that path calls $this->page->SetIsNotFound() — the presenter never
touches SetIsNotFound() at all (confirmed by grep; it's only invoked from
tryBasicAuth()). So the bool return of $this->presenter->PageLoad() is
thrown away, $this->notFound stays false, and execution falls through to
renderFeed() regardless.

Diffed against the prior commit to confirm this is a regression introduced
here, not pre-existing:

// HEAD~1 — Pages/Export/CalendarSubscriptionPage.php
public function PageLoad(): void
{
    if (!$this->presenter->PageLoad()) {
        http_response_code(404);
        return;
    }
    ...
}

That check existed and was correctly wired before this commit moved the
lifecycle into the shared base class. The move dropped it.

Concrete impact: tryBasicAuth() only 404s if the subscription key is
empty. A request with a non-empty but wrong key, or a correct key for a
resource/schedule/user that has calendar subscription explicitly disabled,
now gets HTTP 200 with an empty (but valid) ICS/Atom feed instead of the
intended 404. $this->reservations stays [] because SetReservations() is
never reached, so no reservation data leaks — but it silently breaks the
admin-facing "disable calendar subscription" control and misrepresents
invalid requests as successful empty calendars to calendar clients.

This is untested: the new CalendarSubscriptionPageTest only exercises
tryBasicAuth() in isolation via a hand-rolled TestableSubscriptionPage,
and CalendarSubscriptionPresenterTest only exercises the presenter in
isolation. Nothing drives SubscriptionPage::PageLoad() end-to-end, which is
exactly why this slipped through.

Fix: capture and check the return value, e.g.

if (!$this->presenter->PageLoad()) {
    http_response_code(404);
    return;
}

(and drop the now-redundant SetIsNotFound() plumbing for this path, or keep
it consistent — but the bool check is the load-bearing part that's missing).

Everything else checks out

• Basic Auth design is sound: fail-closed ordering (ICS disabled → 404,
  missing key → 404 before touching credentials, avoiding a credential
  oracle), feedUserSession is correctly request-scoped and never written to
  $_SESSION/cookie, LoginForFeed/BuildSession correctly re-checks
  AccountStatus::ACTIVE and reuses the already-validated credentials without
  a second password check.
• Config wiring (ConfigKeys::ICS_BASIC_AUTH, .env.example,
  config.dist.php) is consistent — verified with
  php scripts/generate-config-dist.php --check and
  generate-env-example.php --check, both pass.
• Widening Authentication::GetUserSession from private to protected is
  only for test subclassing (TestableAuthentication), reasonable.
• IWebAuthentication::LoginForFeed / IAuthentication::BuildSession are
  well-documented as to why they exist (avoid last-login writes + avoid
  session cookie emission on every poll) — matches the commit message.
• Minor/non-blocking: no rate limiting on the Basic Auth attempts on this
  endpoint (unlimited password guesses per request), but this matches prior
  context that a separate "failed login delay" commit is planned next in this
  stack — acceptable to land separately as long as it lands before/with this
  reaching production.
• Commit message is accurate about what was added, just doesn't (and isn't
  expected to) call out the regression above.

Bottom line

The Basic Auth feature itself is well-built, but the base-class refactor
accidentally deletes the existing 404 gate for invalid subscription keys /
disabled subscriptions on both ICS and Atom feeds. That's a real
correctness bug in a security-adjacent code path with no test coverage
catching it — it should be fixed (one-line change) before this merges
upstream, ideally with a regression test that drives
SubscriptionPage::PageLoad() (not just tryBasicAuth()) with a wrong key
and asserts a 404.

[JohnVillalovos]

@lucs7 Feel free to fork this branch and then update it and create a new PR if you want.

Thanks.