Integrate Hanko SSO login without the framework upgrade - #545
Conversation
Adds hotosm-auth alongside the existing OSM OAuth2 login, switchable with AUTH_PROVIDER. Hanko middleware maps the session onto a Django user so login_required and the permission classes keep working unchanged. Installs hotosm-auth without the [django] extra: that extra only pins django>=4.2 and drf>=3.14, which this project already provides at its own versions, so pulling it would force an unrelated framework upgrade. The OAuth2 provider routes stay mounted under both providers -- they are the public API's authorization server for third parties, not part of user login. Also fixes the login view to preserve ?next=, to route unmapped users through onboarding instead of dropping them on the home page, to reject off-site redirect targets, and to url-encode return_to so an OAuth2 target's own query string survives.
Replaces the OAuth2 login button with the shared hotosm-auth component when running under Hanko, and adds the hotosm-tool-menu component. API calls go through buildAuthConfig, which sends cookies under Hanko and keeps the bearer header under the legacy provider.
Covers the auth helpers, the status and onboarding views, and the DRF authentication class.
Compose stack and dev Dockerfile used to run the Hanko branch on export.testlogin.hotosm.org against dev.login. Redis host and the public raw-data-api URL now come from the environment so the containers can reach them, and ADMIN_EMAILS moves out of the compose file into .env. Also fixes a broken condition in the task runner: the SUBMITTED/RUNNING check used or, which is always true.
task_runners has been passing userinfo= to Galaxy since Aug 2024, but the pin stayed at 2.0.10 and no published release accepted that argument until 2.0.17. Deployments have been getting the library from a sibling checkout (see the worker target in the Makefile) or a manual install, so the stale pin went unnoticed; installing exactly what requirements.txt asks for makes every Raw Data API export fail with a TypeError.
tasks/tests/fixtures/all_features_filters.json is read by osm_export_tool at runtime, not just by the test suite, so excluding **/tests/ made every Raw Data API export fail with FileNotFoundError. This is the development Dockerfile, so the suite belongs in the image anyway — make test_all can now run inside the container.
|
Thanks for making this - makes sense to avoid the out of scope upgrades if not needed. Hopefully this is a bit easier to review & debug when we need to fix anything on prod deploy 👍 |
spwoodcock
left a comment
spwoodcock
left a comment
There was a problem hiding this comment.
Much tighter PR focusing only on the auth integration.
Looks good to me:
- I reviewed the backend code.
- The frontend code I skimmed and it looks fine, but there is a lot of view every line.
- As for the tests, were the written post-update, or pre-update? It would be great if these were tests that checked the existing functionality on prod, then we tested that nothing broke in the tests after implementing the auth. No biggie, as the tests are win either way I guess
| ) | ||
|
|
||
| CORS_ALLOW_CREDENTIALS = True | ||
| CORS_ALLOWED_ORIGIN_REGEXES = [ |
There was a problem hiding this comment.
Should these probably be set by env vars, instead of hardcoded as insecure defaults?
When we deploy, we can be sure to set in the .env:
- localhost is fine for the test server
r"^https://.*\.hotosm\.org$",only for prod
There was a problem hiding this comment.
Is this only for local dev too, or the actual production nginx config? Probably worth clarifying in a comment at the top 🙏
If it's prod, we could possibly tighten it up a little with extra headers, compression, caching etc.
| if not user: | ||
| user = job.user | ||
| if job.last_run_status != "SUBMITTED" or job.last_run_status != "RUNNING": | ||
| if job.last_run_status != "SUBMITTED" and job.last_run_status != "RUNNING": |
There was a problem hiding this comment.
Just leaving a comment here to say this was a fix from the previous PR.
The conditional always resulted in True previously, now it doesn't.
The .save can be safely removed as .create automatically calls .save when used.
manage.py test aborts before running anything: Django 3.2 serialises the whole database when creating the test database, through a server-side cursor that the conn_max_age=500 above invalidates mid-iteration, so setup dies with "cursor _django_curs_... does not exist". Nothing here uses serialized_rollback, and Django 4.1 flipped this default for the same reason. make test_all runs after this.
AUTH_PROVIDER=legacy was meant to behave like main, so the branch could be deployed without changing anything for users and Hanko switched on afterwards. Two changes broke that, and both applied unconditionally: api/views.py took login_required from hotosm_auth_django, which checks request.hanko_user — an attribute only HankoAuthMiddleware sets, and that middleware is not installed under legacy. Every decorated view answered 401 to logged-in users: request_nominatim, request_geonames, get_overpass_timestamp, get_overpass_status, get_user_permissions and get_groups. That is the navbar, the permissions and the AOI search. contrib.py dropped SessionAuthentication from the DRF stack, leaving nothing to authenticate the browser's session cookie under legacy. Reads still worked; creating an export or a configuration came back 403, and perform_create now saves request.user, which would be AnonymousUser. login_required now resolves the provider per request and falls back to Django's decorator, and the DRF authenticators depend on the provider too. Verified with a real session on both: legacy answers 200 where it answered 401, anonymous requests redirect to /login/ as on main, and Hanko is unchanged — a Django session alone still does not authenticate there. The tests needed fixing to see any of this. The Hanko ones simulated the middleware without setting request.hanko_user, so they exercised the 401 path they meant to skip, and two legacy ones passed a MagicMock where the ORM needs a real User. Adds TestLoginRequiredPerProvider and TestRestFrameworkAuthenticators so a future change cannot silently take the session out again.
CORS_ORIGIN_ALLOW_ALL was not a leftover default: it arrived in ae465c9 (2017) in the same commit as the OAuth2 provider, so third-party browser apps could call the public API. Narrowing it to an allowlist left them with no Access-Control-Allow-Origin at all, under both providers — so it would have hit them at merge time, before Hanko was even switched on. Hanko does not need it narrowed. The frontend calls its own origin (EXPORTS_API_URL is request.scheme + request.get_host), so its cookies never cross an origin and CORS is not involved; the Hanko web component runs in the page's origin too, and its calls to login are governed by login's CORS, not ours. A Bearer token set by a client's JS is not a CORS credential either, which is why "*" served the public API in the first place. Confirmed on export.testlogin.hotosm.org with CORS open: login, auth/status, auth/me and permissions all succeed, while third-party origins get "*" back, preflight with Authorization included. Keeps django-cors-headers, now pinned: the fork it replaces has had no release since 2018. 4.5.0 is the version this was tested against.
|
@spwoodcock heads-up: three commits landed after your approval, and one of them reverses something that was in the branch when you reviewed it — so it's worth a second look rather than a straight merge. Details are in the The short version:
CORS goes back to open, reverting the allowlist. Neither of these would have been caught by CI: |
find_legacy_user_by_osm_id looked only at provider `openstreetmap-oauth2`, but that name is recent. Checked against a production dump: of 94,461 users, 42,696 are connected under `openstreetmap-oauth2` and 55,163 under the OAuth1-era `openstreetmap` — 51,763 carry the old name only. The uid means the same thing in both (the OSM user id) and has the same shape, numeric, 3 to 8 digits. So 45% of users matched by OSM id and the remaining 55% fell through to the email fallback, which compares the Hanko email against the Export Tool one and only works when they happen to be the same address. When neither matches, onboarding returns no_existing_osm_account and sends the user back to login without an account: 38,902 of those users have exports, and 35 of the 46 owners of OAuth2 applications were in that group. Matching both providers takes it to 94,459 of 94,461. The last two have no OSM connection at all and still have an email on file. 630 uids point at two different users — one account per provider, since (provider, uid) is unique — so this cannot use .get() without raising MultipleObjectsReturned on them. It orders instead: the modern provider first, then the oldest account. All 630 have a row under the modern provider, so each resolves to the account that used the current login. The tests now create real UserSocialAuth rows. The mocked manager they used before could not have caught this: it asserted that some object came back, not which provider was queried.
|
Ran the legacy-onboarding check the description lists under "Not yet verified", against a production dump (94,461 users). It found a real one, now fixed in 7ab09b0.
51,763 accounts carry the old name only, so as far as the lookup was concerned they had no OSM connection at all. They fell through to the email fallback, which compares the Hanko email against the Export Tool one — and when neither matches, 38,902 of those users have exports, and 35 of the 46 owners of OAuth2 applications were in that group. Matching both providers takes it from 42,696 to 94,459 of 94,461. The uid is the OSM user id under both names, same shape (numeric, 3–8 digits). The last two users have no OSM row at all and still have an email on file. One wrinkle: 630 uids point at two different users — one account per provider, since Two other things from the same dump, both good news: no duplicate emails among 94,461 users, so the email fallback never merges two people; and no user without an email, so nobody is left without any matching path. The tests now create real Still unverified: a real legacy user completing the flow end to end against Hanko. That needs a test account on the login service with OSM connected. |
The middleware set is_superuser from ADMIN_EMAILS and left is_staff alone, but Django's admin gates on is_staff — `AdminSite.has_permission` returns `request.user.is_active and request.user.is_staff`. So a listed admin held every permission and still could not open the admin. The two halves disagreed in the worst way. The navbar checks `auth.add_user`, which is_superuser satisfies on its own, so the Admin entry appeared; clicking it went /admin/ -> /admin/login/?next=/admin/ -> /login/ -> /v3/ and landed back on the home page with nothing explaining why. Confirmed on the test deployment with an account that is in ADMIN_EMAILS and has no is_staff row. An is_superuser without is_staff is not a state Django produces on its own — createsuperuser sets both — so this only ever appeared through the middleware. is_staff is now granted the same way, keeping whatever the database already had: five production users have is_staff without being superusers, and that is their admin access. `is_admin or django_user.is_staff` adds to it rather than replacing it. Nothing is persisted, as before: both flags live for the request only.
2 participants
Integrates the shared HOT login (Hanko SSO) into the Export Tool, as an
alternative to #538.
This branch builds directly on @andrea-chirillano's work in #538 — the auth
helpers, middleware, onboarding flow, frontend integration and tests all come
from there. What changes is the dependency footprint and a handful of fixes
found while reviewing it.
Why a second PR
#538 upgrades the framework along with the login change:
~=3.2.22→~=4.2~=3.2.22(unchanged)1.3.2→>=2.0.01.3.2(unchanged)~=3.11.0→>=3.14.0~=3.11.0(unchanged)~=0.15→>=1.0~=0.15(unchanged)The framework upgrade turned out not to be necessary.
hotosm-auth[django]declares
django>=4.2anddrf>=3.14, but that's the extra's pin, not arequirement of the code:
hotosm_auth_djangoonly importsAppConfig,settings,connection,JsonResponse,SimpleLazyObject,csrf_exempt,pathand DRF'sAPIView/Response/status— all present in 3.2. Installinghotosm-authwithout the extra pulls the same wheel, and Django and DRFstay at the versions this project already provides.
The practical difference is the database. django-oauth-toolkit 1.3.2 ships 2
migrations; 2.x ships 13. Production is on those same 2. #538 would apply 11
schema migrations to the OAuth2 tables, which a revert doesn't undo. This
branch applies exactly one new migration,
hotosm_auth_django.0001, whichcreates a table and touches nothing existing.
Fixes on top of #538
get_user_permissionsreturned[]for every non-superuser under Hanko.It only read the superuser branch and never looked at
user_permissionsorgroups, so users with HDX or partner rights lost those menus. Both providers
now resolve permissions the same way — the middleware maps the session onto a
real Django user, so per-user and group permissions still apply.
:has()block with!importanthid HDX, Partners, Admin, Workers, Status and Stats foreveryone under Hanko. It was compensating for the empty permissions above;
with those fixed it isn't needed, and it would have overridden them anyway.
/o/under Hanko,which takes down the public API's authorization server — the application
console and token revocation that Help → API links to. Those routes are not
part of user login and now stay mounted under both providers.
loginview: preserves?next=, routes users without a mapping throughonboarding instead of dropping them on the home page, rejects off-site
redirect targets, and url-encodes
return_toso an OAuth2 target's own querystring survives (an unescaped
&truncated it).ADMIN_EMAILSmoved out of the compose file into.env, next toSECRET_KEYandCOOKIE_SECRET. Changing who is an admin no longer needs acommit.
!= "SUBMITTED" or != "RUNNING"is always true; nowand.Also left out of this branch, as unrelated to login: the test-fixture changes
(
conftest.py,setup.cfg,FeatureSelection→ constant), ayarn.lockthatmoved without
package.json(addinglit,preact,classnames— nonedeclared or imported, the components load from CDN),
requireAuthon/exportsand/configurationswhich are public routes on main, and anavigation change on configuration save.
Tested on export.testlogin.hotosm.org
Deployed from this branch against dev.login, with a clean database.
django 3.2.25,drf 3.11.2,DOT 1.3.2.oauth2_providermigrations: 2, same as production.those 6 and
is_superuser: false, and sees HDX and Partners in the navbar —while Admin, Workers, Status and Stats stay hidden, since those need
auth.add_user.ADMIN_EMAILSgrants superuser per request without persisting it to thedatabase.
/o/applications/, authorized it through the Hanko login, and the resultingbearer token returned that user's private configuration from
/api/configurations— while no token, an invalid token and an expired tokenall returned nothing. Hanko authenticates people, OAuth2 keeps authorizing
machines.
Behaviour changes worth knowing
/api/permissionsreturns 401 instead of a 302 to the login page,and some DRF endpoints return 403 where they returned 401 (the Hanko
authenticator doesn't set a
WWW-Authenticateheader). Both still deny;clients that follow the redirect or distinguish the codes are affected.
CORS_ORIGIN_ALLOW_ALLto a regex allowlist(
*.hotosm.org,*.hotosm.test, localhost) — credentialed CORS can't useallow-all.
ADMIN_EMAILSoverridesis_superuserper request. It must be populatedbefore switching a deployment to Hanko, or current admins lose access.
Not yet verified
from an empty database. Production has users with
social_authrecords, andonboarding matches them by OSM id and falls back to email. This needs a run
against a production dump before going live — including that their existing
OAuth2 applications stay attached to the same Django user.
ci.yamlrunsmake test,which is a single HDX test file. The suite is under the
test_alltarget.the test environment; the production deploy needs its own env wiring.
AUTH_PROVIDERdefaults tolegacy, and in that mode this branch behaves likemain — so it can be deployed without changing anything for users, and Hanko
switched on afterwards with one variable.
Update — three commits added after the approving review
Testing the claim in the paragraph right above this section showed it wasn't
true, so please re-read that paragraph as: it is true now, it wasn't when this
was reviewed. Two changes in this branch applied unconditionally, not just
under Hanko, and both broke
AUTH_PROVIDER=legacy.Keep the legacy auth provider workingapi/views.pytooklogin_requiredfromhotosm_auth_django. That decoratorchecks
request.hanko_user, an attribute onlyHankoAuthMiddlewaresets — andunder legacy that middleware isn't installed. Every decorated view answered 401
to logged-in users:
request_nominatim,request_geonames,get_overpass_timestamp,get_overpass_status,get_user_permissionsandget_groups. That's the navbar, the permissions and the AOI search.core/settings/contrib.pyalso droppedSessionAuthenticationfrom the DRFstack, so nothing authenticated the browser's session cookie under legacy.
Reads still worked; creating an export or a configuration returned 403 — and
perform_createnow savesrequest.user, which would beAnonymousUser.Same request, a real Django session, before and after:
GET /api/permissions401200with username and permissionsGET /api/groups401200GET /api/overpass_status401200POST /api/configurations403201GET /api/permissions(anonymous)401302 → /login/(as on main)login_requirednow resolves the provider per request and falls back toDjango's decorator; the DRF authenticators depend on the provider too. Hanko
is byte-for-byte unchanged, including that a Django session alone still does
not authenticate there.
Keep CORS open for third-party API clientsThis one reverses a decision that was in the branch when it was reviewed.
CORS_ORIGIN_ALLOW_ALLisn't a leftover default: it arrived in ae465c9 (2017)in the same commit as the OAuth2 provider, so third-party browser apps could
call the public API. Narrowing it to an allowlist left them with no
Access-Control-Allow-Originat all — and under both providers, so it wouldhave hit them at merge time, before Hanko was switched on.
The justification in
requirements.txt("Hanko needs credentialed CORS") doesnot hold. The frontend calls its own origin —
EXPORTS_API_URLisrequest.scheme + request.get_host— so its cookies never cross an origin andCORS never participates. The Hanko web component is fetched from login but runs
in the page's origin, so its calls to login are governed by login's CORS, not
ours. And a Bearer token set by a client's JS is not a CORS credential, which is
why
*served the public API in the first place.Verified on export.testlogin.hotosm.org with CORS open: login,
auth/status,auth/meandpermissionsall 200, navbar complete, while third-party originsget
*back, preflight withAuthorizationincluded.django-cors-headersstays — the fork it replaces has had no release since2018 — now pinned to 4.5.0, the version this was tested against.
Let the test suite build its databasemanage.py testaborted during setup on Django 3.2: it serialises the wholedatabase through a server-side cursor that
conn_max_age=500invalidatesmid-iteration.
make test_allruns after this.That mattered, because the tests needed fixing before they could show any of the
above. The Hanko tests simulated the middleware without setting
request.hanko_user, so they exercised the 401 path they meant to skip, and twolegacy ones passed a
MagicMockwhere the ORM needs a realUser. Nine testswere failing; all pass now, under
AUTH_PROVIDER=legacyandhanko. Two newclasses,
TestLoginRequiredPerProviderandTestRestFrameworkAuthenticators,keep the session from being silently removed again.
Still red, and inherited from
main(confirmed by running the suite at themerge base):
api.tests.test_views,jobs.tests.test_modelsandtasks.tests.test_modelsfail to import —feature_selectionis a moduleosm-export-tool-python 2.x no longer ships — plus 3 failures in
api.tests.test_filters.Two things found while testing, unrelated to login
auth_group.is_partnerhas no migration. It's added viaGroup.add_to_class()injobs/models.py:39, which Django doesn't migrate,so any fresh database lacks it and
/api/groupsreturns 500 — the Partnersmenu was broken on testlogin for that reason. Production has the column
because it was created by hand. Worth a real migration.
api-stage.raw-data.hotosm.orgserves a certificate issued forkshitij-dev.hotosm.org, so no export can complete against stage(
SSLError). Production's host is fine (CN=*.raw-data.hotosm.org) — butnote
RAW_DATA_API_URLdefaults to stage inproject.py, so a deploymentthat forgets the variable would silently point at the broken host.