fix(solicitations): require login on marketplace views (closes #198)

[jjackson]

Product Description

The /solicitations/ marketplace listing and /solicitations/<id>/ detail pages now require a labs login. Anonymous visitors are redirected to /labs/login/?next=… and land back on the page they wanted after they auth.

Before this change, anonymous visitors saw a misleading empty "No solicitations available" listing and 404s on detail pages, even though the records existed and were marked is_public=true. Logged-in users were unaffected.

Technical Summary

Closes #198.

The previous _get_public_data_access helper in commcare_connect/solicitations/views.py tried to fall back to a CLI OAuth token loaded from ~/.commcare-connect/token.json whenever the request had no session. That file is a dev-time convenience — nothing in Dockerfile, docker/start, the ECS task definition, or the deploy workflow puts one on the production container. The fallback therefore raised `ValueError` from `SolicitationsDataAccess(access_token=None)`, which got swallowed by the views' broad `except Exception` and rendered an empty listing (`PublicSolicitationListView`) or a 404 (`PublicSolicitationDetailView`) — silently, with the failure visible only in server logs.

The deeper reason the fallback couldn't work even in principle: production's `LabsRecordDataView` (`dimagi/commcare-connect`) declares `permission_classes = [IsAuthenticated, TokenHasScope]` with `required_scopes = ["export"]` on every request. The `public=True` filter on `LabsRecord` is an ACL on which records get returned, not an auth-bypass on the endpoint. Any caller — including `labs` rendering an anonymous marketplace — needs a real OAuth bearer token.

So instead of building a service-account token path in labs (which would require designating a user, configuring an env var, and re-using the existing `get_valid_access_token` / refresh-token plumbing), this PR honestly admits that the marketplace requires a login. The two views gain `LabsLoginRequiredMixin` and use the regular `_get_data_access` helper. The dead `_get_public_data_access` function is removed.

Class names (`PublicSolicitationListView`, `PublicSolicitationDetailView`) and URL names (`public_list`, `public_detail`) are left as-is to avoid downstream churn in `mcp_tools.py`, the ACE plugin (which links to these URLs in solicitation invite emails), and any human-authored references. Only the visibility model changes; the surface keeps its name.

Safety Assurance

Safety story

The change is strictly narrowing: every previously-anonymous request now redirects to `/labs/login/` instead of being served. There is no behavior change for authenticated users — they go through the same `_get_data_access(request)` codepath the manage views use, and `get_public_solicitations()` returns the same `public=True` records as before.

Two ways this could go wrong:

1. A consumer was actually relying on the anonymous endpoint. Verified the only inbound callers: ACE invite emails (link to detail URLs, where recipients always need to log in to respond anyway), the labs prelogin marketing page (no link to `/solicitations/`), and direct browser visitors (the case in Public /solicitations/ listing renders empty + detail URLs 404 anonymously despite is_public=true on envelope #198, who already saw a broken empty page). MCP and `api_views.py` already require auth and are untouched.
2. The redirect loop traps a logged-in-but-token-expired user. The Django session is the gate; if it's valid, `LabsLoginRequiredMixin` lets the request through. The expired-`labs_oauth` case (Django session OK, OAuth token expired) still falls into the same silent-empty-list path that exists today on the manage views — pre-existing behavior, separate ticket if it bites someone.

Automated test coverage

• `commcare_connect/solicitations/tests/test_public_views_auth.py` — two new tests that pin the redirect contract: anonymous GET on `/solicitations/` and `/solicitations//` both return 302 to `/labs/login/?next=…` with the original URL preserved.
• Full `commcare_connect/solicitations/` suite (172 tests) passes locally; nothing references `_get_public_data_access` outside the deleted block.

QA Plan

On labs, post-deploy:

1. Open `https://labs.connect.dimagi.com/solicitations/\` in a fresh incognito window. Expect a redirect to `/labs/login/?next=/solicitations/` instead of the empty "No solicitations available" card.
2. Log in via the labs flow. Expect to land back on `/solicitations/` and see the four `is_public=true` records (3122, 2288, 2505, 2227) referenced in Public /solicitations/ listing renders empty + detail URLs 404 anonymously despite is_public=true on envelope #198.
3. Open one of them directly (e.g. `/solicitations/3122/`) anonymously. Expect a redirect to login; after login, expect a 200 with the detail page.

Labels & Review

• The set of people pinged as reviewers is appropriate for the level of risk of the change

🤖 Generated with Claude Code

[github-actions]

🤖 AI Code Review

Code Review

Summary

This PR removes a broken CLI token fallback mechanism and makes marketplace views explicitly require authentication. The changes are well-documented and the tests correctly verify the new behavior.

---

Findings

🔵 Suggestion - Test robustness: Handle malformed query strings

File: commcare_connect/solicitations/tests/test_public_views_auth.py (lines 27, 40)

next_param = dict(p.split("=", 1) for p in parts.query.split("&"))["next"]

This will raise a ValueError if any query parameter lacks an = sign. Consider:

from urllib.parse import parse_qs
next_param = parse_qs(parts.query)["next"][0]

Or add error handling to make the test intent clearer if malformed URLs are encountered.

---

🔵 Suggestion - Test coverage: Verify authenticated access still works

File: commcare_connect/solicitations/tests/test_public_views_auth.py

The tests verify that anonymous users are redirected, but there's no test confirming that authenticated users can still access these views. Consider adding a test like:

@pytest.mark.django_db
def test_public_list_works_for_authenticated_user(authenticated_client):
    url = reverse("solicitations:public_list")
    resp = authenticated_client.get(url)
    assert resp.status_code == 200

This ensures the happy path still works after the refactoring.

---

🔵 Suggestion - Error handling consideration

File: commcare_connect/solicitations/views.py (lines 288-296, 307-310)

The try/except blocks catch Exception which previously made sense when falling back to CLI tokens. Now that authentication is required, these broad exception handlers might mask legitimate errors. Consider:

1. Being more specific about caught exceptions (e.g., LabsAPIError)
2. Logging the actual exception before showing the generic error message
3. Re-evaluating whether these try/except blocks are still needed or if errors should propagate

---

Verdict

✅ Code looks good overall. The refactoring correctly removes dead code and makes authentication requirements explicit. The tests properly verify the new redirect behavior. The suggestions above would improve robustness but aren't blockers.

---

Powered by Claude — auto-generated review