nycmeshnet / meshdb

A convenient, stable, and sane database for tracking Members and Nodes for use with robots and humans
https://db.nycmesh.net
MIT License
9 stars 13 forks source link

Ensure that InstallList view (and other views) don't recurisvely resolve member details #61

Closed WillNilges closed 10 months ago

WillNilges commented 1 year ago

https://github.com/andybaumgar/meshdb/blob/90e51008b810cb1c305e8950396c85f0a7354263/src/meshapi/views.py#L67

quoncc commented 1 year ago

The install view looks alright. I set up a dummy instance with 2 installs, 2 buildings, and 2 members and logged all queries to the DB.

Below is what appears in the log after loading the InstallList view

2023-09-15 01:13:22.681 UTC [119] LOG: connection received: host=172.17.0.1 port=59862 2023-09-15 01:13:22.708 UTC [119] LOG: connection authenticated: identity="nycmesh" method=scram-sha-256 (/var/lib/postgresql/data/pg_hba.conf:100) 2023-09-15 01:13:22.708 UTC [119] LOG: connection authorized: user=nycmesh database=nycmesh-dev 2023-09-15 01:13:22.719 UTC [119] LOG: statement: SELECT set_config('TimeZone', 'UTC', false) 2023-09-15 01:13:22.722 UTC [119] LOG: statement: SELECT "django_session"."session_key", "django_session"."session_data", "django_session"."expire_date" FROM "django_session" WHERE ("django_session"."expire_date" > '2023-09-15 01:13:22.664621+00:00'::timestamptz AND "django_session"."session_key" = '7iv6qclp8ya7ole94tk4ed6295iidp8a') LIMIT 21 2023-09-15 01:13:22.729 UTC [119] LOG: statement: SELECT "auth_user"."id", "auth_user"."password", "auth_user"."last_login", "auth_user"."is_superuser", "auth_user"."username", "auth_user"."first_name", "auth_user"."last_name", "auth_user"."email", "auth_user"."is_staff", "auth_user"."is_active", "auth_user"."date_joined" FROM "auth_user" WHERE "auth_user"."id" = 1 LIMIT 21 2023-09-15 01:13:22.734 UTC [119] LOG: statement: SELECT COUNT(*) AS "__count" FROM "meshapi_install" 2023-09-15 01:13:22.737 UTC [119] LOG: statement: SELECT "meshapi_install"."id", "meshapi_install"."install_number", "meshapi_install"."install_status", "meshapi_install"."building_id_id", "meshapi_install"."member_id_id", "meshapi_install"."install_date", "meshapi_install"."abandon_date" FROM "meshapi_install" LIMIT 2 2023-09-15 01:13:22.758 UTC [119] LOG: statement: SELECT "meshapi_building"."id", "meshapi_building"."bin", "meshapi_building"."building_status", "meshapi_building"."street_address", "meshapi_building"."city", "meshapi_building"."state", "meshapi_building"."zip_code", "meshapi_building"."latitude", "meshapi_building"."longitude", "meshapi_building"."altitude", "meshapi_building"."network_number", "meshapi_building"."install_date", "meshapi_building"."abandon_date" FROM "meshapi_building" LIMIT 1000 2023-09-15 01:13:22.762 UTC [119] LOG: statement: SELECT "meshapi_member"."id", "meshapi_member"."first_name", "meshapi_member"."last_name", "meshapi_member"."email_address", "meshapi_member"."phone_numer", "meshapi_member"."slack_handle" FROM "meshapi_member" LIMIT 1000 2023-09-15 01:13:22.768 UTC [119] LOG: disconnection: session time: 0:00:00.088 user=nycmesh database=nycmesh-dev host=172.17.0.1 port=59862

Looks like it selects all from each table just once.

quoncc commented 1 year ago

Whoops, reading the last issue for context, I now realize I misunderstood. I thought we were worried about recursive DB calls for performance reasons - not for security reasons.

So yes, calling the install view does query member data as well. However, it doesn't look like this data gets passed onto the client (at least from what I can gather F12ing the install view)

We can chat more about this during our next congregation.

WillNilges commented 11 months ago

Seems like doing a CURL as unauthenticated will give you the IDs, but then as an unauthenticated user, you can't really do anything with those IDs, because the routes are not available to you.

The only place this may be of a concern is the default UI, but that's not going to be a production thing. Right now it will 403 you, but if you add the permissions.IsAuthenticatedOrReadOnly permission_class, it works again. This is because the UI has all kinds of GETs and POSTs and what have you to make.

TL;DR: Probably not a big deal.