Repository navigation
feat(geo): record created_by / profile and authorize the owner - #473
Merged
Merged
Conversation
…ate tiles `get_obj_from_relay_id` resolves a public ID through DocumentId and returns the object of ANY registered model. The geo mutations used it without a type check, so a user holding only the geo model permissions could reach other blocks' rows: - geoFeatureDelete called obj.delete() on whatever came back. A User's (or Comment's, Follow's, Rate's) relay ID deleted that row. - geoFeatureUpdate passed it to GeoJSONFeatureForm(instance=obj). With all four optional inputs supplied, construct_instance wrote `name` and `description` onto the foreign instance and form.save() persisted it. Both now resolve through _get_feature(), which type-checks and raises `not_found`. The resolver's own exceptions are caught and logged rather than surfaced, since they leak which IDs exist. geoFeatureCreate rejects an unresolvable targetObjectId the same way, instead of failing later with an IntegrityError on the NOT NULL target_document_id. Also: - Tiles sent `Cache-Control: public` unconditionally, even though the view's permission is overridable precisely so projects can hide features from anonymous users. Visibility is now derived from whether an anonymous user may view, and Vary: Authorization is set so token-auth deployments aren't shared-cached on the session cookie alone. - baseapp/geo/graphql/__init__.py re-exported the ObjectTypes, so importing the FilterSet from the REST tile view pulled in graphene, graphene_django, query_optimizer and baseapp_auth.graphql — making the REST surface depend on the `graphql` extra and defeating the laziness filters.py documents. Emptied, matching baseapp/files and baseapp/content_feed. Not addressed here: object-level authorization on update/delete (no owner column exists on AbstractGeoJSONFeature yet) and the generic `GeometryObjectType` type name. baseapp/geo/tests: 114 passed. black, isort and flake8 clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fczpSCpN3kCRs3UBjbity
AbstractGeoJSONFeature had no owner column, so object-level authorization was impossible: a single group grant of change/delete_geojsonfeature let a user edit or delete every feature in the system. Modelled on baseapp.files. - `created_by` (FK to AUTH_USER_MODEL, null/blank) and, when baseapp_profiles is installed, `profile` (FK to the swappable Profile, SET_NULL) via the same conditional mixin baseapp.files uses. geoFeatureCreate records both — the profile from the request's current_profile, so the Current-Profile header is honoured like everywhere else. - GeoPermissionsBackend grows `_is_owner_or_has_global` for change/delete: the creator may act on their own feature, anyone else still needs the model permission, so moderators are unaffected. With no obj it keeps returning False, leaving ModelBackend to decide the global grant. - geoFeatureUpdate / geoFeatureDelete now resolve the feature BEFORE authorizing, so the check gets the object instead of None. - Admin shows and autocompletes the new columns, and select_relates created_by. `created_by` is nullable and `AnonymousUser.pk` is also None, so a plain `created_by_id == user.pk` makes every anonymous user the owner of every unowned feature. The backend requires `is_authenticated` before the comparison; a test pins it. Migration: testproject/geo/0002. Consuming projects with their own concrete model need their own AddField migration. baseapp/geo/tests: 121 passed. makemigrations --check clean. black, isort, flake8 clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fczpSCpN3kCRs3UBjbity
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fczpSCpN3kCRs3UBjbity # Conflicts: # baseapp/geo/graphql/mutations.py # baseapp/geo/tests/test_graphql_mutations.py
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nossila
added a commit
that referenced
this pull request
Oct 9, 2026
Still-valid findings from the Copilot and CodeRabbit reviews. Four others were already fixed by #472 / #473 and are answered on the threads rather than here. - Permission codenames were hardcoded to `geojsonfeature`. The model is swappable, so a project swapping in `MapFeature` gets `add_mapfeature` and none of the checks would ever match. All of them now derive the codename from `_meta.model_name`, across permissions, mutations, object types and the REST view/permission. - `geoFeatures` never enforced the read permission. DjangoConnectionField resolves edges straight from the queryset and never calls `get_node`, so a project that overrides the public-read backend still had every feature listable. Added `get_queryset`, returning `skip_ast_walker(queryset.none())` so the optimizer isn't run over an empty queryset. - The legacy base64 branch of the target filter validated the type name but never checked the row exists, so a well-formed ID for a deleted object returned an empty list instead of the documented validation error. It now mirrors the public-id branch. - README: `baseapp_geo` -> `baseapp.geo` in the INSTALLED_APPS instruction (the label is not importable), the auth-backend slot is `baseapp_geo` not `baseapp.geo`, and the factory lives at `baseapp.geo.tests.factories`. baseapp/geo/tests: 124 passed. black, isort and flake8 clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fczpSCpN3kCRs3UBjbity
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review findings 3 and 4 — the ones that needed a schema decision. Modelled on
baseapp.files.Problem
AbstractGeoJSONFeaturehad no owner column, so there was nothing to authorize against.GeoPermissionsBackendreturnedFalsefor change/delete, which handed the whole decision toModelBackend's model-level grant — one group membership let a user edit or delete every feature in the system, including features attached to other users' objects.Changes
created_by— FK toAUTH_USER_MODEL,null=True, blank=True,related_name="geo_features_created".profile— FK to the swappable Profile withon_delete=SET_NULL, added through the sameapps.is_installed("baseapp_profiles")conditional mixinbaseapp.filesuses, so the block still works without that package.geoFeatureCreaterecords both. The profile comes from the request'scurrent_profile, so theCurrent-Profileheader is honoured the way it is everywhere else.GeoPermissionsBackend._is_owner_or_has_globalfor change/delete: the creator may act on their own feature; anyone else still needs the model permission, so moderators are unaffected. With noobjit still returnsFalse, leavingModelBackendto decide the global grant.geoFeatureUpdate/geoFeatureDeleteresolve the feature before authorizing, sohas_permreceives the object rather thanNone.select_relatedscreated_by.One trap worth calling out
created_byis nullable, andAnonymousUser.pkis alsoNone— so the obviousobj.created_by_id == user.pkmakes every anonymous user the owner of every unowned feature. I hit exactly this: anonymous update/delete started succeeding andhasPermreturnedcanChange: truefor anonymous. The backend now requiresis_authenticatedbefore the comparison, andtest_anonymous_is_not_the_owner_of_an_unowned_featurepins it.(
baseapp.filessidesteps it by comparing instances —obj.created_by == user_obj, whereNone == AnonymousUser()is False. Comparing ids avoids an FK fetch, but is only safe behind the auth guard.)created_byalso needsblank=True, not justnull=True, orfull_clean()rejects every feature created without one — which the model and admin tests caught.Testing
baseapp/geo/tests: 121 passed.makemigrations --checkreports no changes. black, isort, flake8 clean.New coverage: create records user + profile; owner updates/deletes their own feature with no model permission; non-owner is denied and the row is verifiably unchanged; a holder of the global permission still reaches other users' features; anonymous is not an owner.
Follow-up required in the template
baseapp-backend-templatehas its own concrete model (apps/social/geo) and needs its ownAddFieldmigration once the submodule pointer is bumped. Any other consuming project does too — called out in the commit message.🤖 Generated with Claude Code
https://claude.ai/code/session_015fczpSCpN3kCRs3UBjbity