diff --git a/src/django_smartbase_admin/actions/admin_action_list.py b/src/django_smartbase_admin/actions/admin_action_list.py index ee2b9eaf..63bda1a4 100644 --- a/src/django_smartbase_admin/actions/admin_action_list.py +++ b/src/django_smartbase_admin/actions/admin_action_list.py @@ -832,11 +832,16 @@ def get_aggregates(self, aggregate, field_map=None, group_by=None): degenerate case of the same query. Without ``group_by`` returns a scalar ``{alias: value}`` dict; with - ``group_by`` returns a list of ``{**group_fields, **aliases}`` rows, - ordered by the group columns. Each metric runs in its own query and - is merged by group key, so a metric that traverses a to-many - relation (a multi-valued ``count``, or an annotated field that - aggregates a related set) can never fan-out-inflate a sibling. + ``group_by`` returns a list of + ``{"group": {name: value, ...}, "aggregates": {alias: value, ...}}`` + rows, ordered by the group columns. The two namespaces are nested + rather than merged into one flat dict so a group column whose public + name equals a derived aggregate alias (e.g. a ``count`` field grouped + on while also aggregating ``count``) can't clobber the other — they + live under separate keys. Each metric runs in its own query and is + merged by group key, so a metric that traverses a to-many relation (a + multi-valued ``count``, or an annotated field that aggregates a related + set) can never fan-out-inflate a sibling. """ specs = self.validate_aggregate(aggregate, field_map=field_map) group_fields = self.validate_group_by(group_by, field_map=field_map) @@ -864,11 +869,20 @@ def get_aggregates(self, aggregate, field_map=None, group_by=None): # Resolve group targets the same way; a computed (annotated) group # column needs its expression present before ``.values()``. + # Keep the caller-facing group key (the declared name they grouped by) + # separate from the ORM target. They diverge when a declared field's + # public name differs from its DB column — e.g. an ``id_alias`` method + # field backed by ``@admin.display(ordering="id")`` resolves to ``id``. + # ``group_targets`` drives ``.values()`` / ``order_by`` / dedup; the + # response ``group`` dict is keyed by ``group_keys`` so + # ``group_by=["id_alias"]`` comes back under ``id_alias``, not ``id``. + group_keys: list[str] = [] group_targets: list[str] = [] for field in group_fields: target = ( field.model_field.name if field.model_field is not None else field.field ) + group_keys.append(field.name) group_targets.append(target) if ( field.model_field is None @@ -878,6 +892,7 @@ def get_aggregates(self, aggregate, field_map=None, group_by=None): if field.supporting_annotates: extra.update(field.supporting_annotates) extra[target] = field.annotate + if extra: queryset = queryset.annotate(**extra) @@ -909,13 +924,20 @@ def get_aggregates(self, aggregate, field_map=None, group_by=None): key = tuple(row[t] for t in group_targets) slot = merged.get(key) if slot is None: - slot = {t: row[t] for t in group_targets} + slot = { + "group": { + key_name: row[target] + for key_name, target in zip(group_keys, group_targets) + }, + "aggregates": {}, + } merged[key] = slot order.append(key) - slot[alias] = row[alias] + slot["aggregates"][alias] = row[alias] except FieldError as exc: raise ValueError(f"Cannot aggregate the requested field(s): {exc}") from exc + # No group dimension → unwrap to the bare scalar aggregate dict. if not group_targets: - return merged[order[0]] if order else {} + return merged[order[0]]["aggregates"] if order else {} return [merged[key] for key in order] diff --git a/src/django_smartbase_admin/mcp/mcp.py b/src/django_smartbase_admin/mcp/mcp.py index 00b83067..0f414d7e 100644 --- a/src/django_smartbase_admin/mcp/mcp.py +++ b/src/django_smartbase_admin/mcp/mcp.py @@ -565,18 +565,22 @@ def list_rows( ``fn`` is one of ``sum / avg / min / max / count``; ``field`` must be a declared column from ``list_admins`` — ``sum/avg/min/max`` need a numeric one, ``count`` may omit - ``field`` for a row count. Without ``group_by`` the results - land under ``aggregates``, keyed ``f"{fn}_{field}"`` (or - ``"count"``). + ``field`` for a row count. Each result is keyed by a derived + alias — ``f"{fn}_{field}"`` (or ``"count"`` for a bare count); + aliases can't be overridden. Without ``group_by`` the results + land under ``aggregates`` as a flat ``{alias: value}`` dict. group_by: optional list of declared column names to break the ``aggregate`` totals down by (SQL ``GROUP BY``) — e.g. ``group_by=["queue"]`` with ``aggregate=[{"fn": "count"}]`` for tickets-per-queue in one call instead of one call per queue. Multi-valued (relation) columns are rejected. Requires ``aggregate``. Results land under ``groups`` as a list of - ``{**group_columns, **aggregate_aliases}`` rows ordered by the - group columns; a column grouped on a foreign key comes back as - the bare pk (resolve names with ``autocomplete``). + ``{"group": {column: value, ...}, "aggregates": {alias: value, + ...}}`` rows ordered by the group columns — group columns and + aggregate aliases are nested under separate keys so a column whose + name equals an aggregate alias can't collide. A column grouped on a + foreign key comes back as the bare pk (resolve names with + ``autocomplete``). Returns ``{"data": [...], "last_page": int, "last_row": int}`` plus any pagination metadata the list view emits, ``aggregates`` diff --git a/src/django_smartbase_admin/mcp/oauth/validators.py b/src/django_smartbase_admin/mcp/oauth/validators.py index 1a8fc448..96a508a5 100644 --- a/src/django_smartbase_admin/mcp/oauth/validators.py +++ b/src/django_smartbase_admin/mcp/oauth/validators.py @@ -31,20 +31,27 @@ def is_non_loopback_http(redirect_uri: str) -> bool: def loopback_redirect_allowed(redirect_uri: str, allowed_uris) -> bool: - """RFC 8252 §7.3 — match a loopback redirect URI ignoring its port. + """RFC 8252 §7.3 — match a loopback ``http`` redirect URI ignoring its port. Loopback hosts are treated as interchangeable (a client registered with - ``127.0.0.1`` may come back as ``localhost``); scheme, path and query must - still match a registered loopback URI exactly. + ``127.0.0.1`` may come back as ``localhost``); path and query must still + match a registered loopback URI exactly. + + Port relaxation is confined to ``http`` — the only RFC 8252 §7.3 case and + the only scheme this validator deliberately permits for loopback hosts. + Custom/native schemes (``cursor://``, ``claude://``) stay on DOT's exact + match, which enforces ``ALLOWED_REDIRECT_URI_SCHEMES``; without this gate a + dynamically registered URI like ``javascript://localhost:1/callback`` could + slip past the scheme allowlist on a mere port mismatch. """ requested = urlsplit(redirect_uri) - if requested.hostname not in LOOPBACK_HOSTS: + if requested.scheme != "http" or requested.hostname not in LOOPBACK_HOSTS: return False for allowed in allowed_uris: candidate = urlsplit(allowed) if ( - candidate.hostname in LOOPBACK_HOSTS - and candidate.scheme == requested.scheme + candidate.scheme == "http" + and candidate.hostname in LOOPBACK_HOSTS and candidate.path == requested.path and candidate.query == requested.query ): diff --git a/src/django_smartbase_admin/mcp/tests/test_aggregate.py b/src/django_smartbase_admin/mcp/tests/test_aggregate.py index ec721dbe..21773471 100644 --- a/src/django_smartbase_admin/mcp/tests/test_aggregate.py +++ b/src/django_smartbase_admin/mcp/tests/test_aggregate.py @@ -25,7 +25,7 @@ class _Admin(SBAdmin): search_fields = ("name",) # ``children`` is the reverse of the self-FK ``parent`` → a one-to-many # (multi-valued) field, used to exercise the join fan-out path. - sbadmin_list_display = ("id", "name", "id_alias", "parent", "children") + sbadmin_list_display = ("id", "name", "id_alias", "count", "parent", "children") # A method field with ``admin_order_field`` → backed by the ``id`` # column via an annotation, so its ORM identifier is suffixed @@ -34,6 +34,13 @@ class _Admin(SBAdmin): def id_alias(self, obj): return obj.id + # A method field whose public name equals the derived ``count`` aggregate + # alias — used to exercise that nesting keeps a group column and an + # equally-named aggregate from colliding. + @admin.display(ordering="id") + def count(self, obj): + return obj.id + @override_settings( ROOT_URLCONF=__name__, @@ -124,16 +131,67 @@ def test_group_by_breaks_totals_down_per_group(self): # No top-level scalar aggregates; a "groups" list instead, one row # per parent, ordered by the group column (NULLs first in sqlite). + # Group columns and aggregates are nested under separate keys. self.assertNotIn("aggregates", result) self.assertEqual( result["groups"], [ - {"parent": None, "count": 2, "sum_id": a.pk + b.pk}, - {"parent": a.pk, "count": 2, "sum_id": sum(a_ids)}, - {"parent": b.pk, "count": 1, "sum_id": sum(b_ids)}, + { + "group": {"parent": None}, + "aggregates": {"count": 2, "sum_id": a.pk + b.pk}, + }, + { + "group": {"parent": a.pk}, + "aggregates": {"count": 2, "sum_id": sum(a_ids)}, + }, + { + "group": {"parent": b.pk}, + "aggregates": {"count": 1, "sum_id": sum(b_ids)}, + }, ], ) + def test_group_by_keys_rows_by_requested_name_not_orm_target(self): + # ``id_alias`` is a method field whose ORM identifier is the annotation + # alias ``id_alias_annt``. Grouping by it must still surface rows under + # the agent-facing name ``id_alias`` (what the caller asked for), not + # the internal target — otherwise the group key the caller reads back + # doesn't match the field they grouped on. + ids = sorted(Folder.objects.create(name=n).pk for n in ("a", "b", "c")) + + result = self._tools().list_rows( + "filer_folder", + fields=["id", "name"], + group_by=["id_alias"], + aggregate=[{"fn": "count"}], + ) + + self.assertNotIn("aggregates", result) + self.assertEqual( + result["groups"], + [{"group": {"id_alias": pk}, "aggregates": {"count": 1}} for pk in ids], + ) + + def test_group_key_equal_to_aggregate_alias_does_not_collide(self): + # ``count`` is a method field whose public name equals the derived + # ``count`` aggregate alias. Nesting the group column and the aggregate + # under separate keys means they no longer share a dict, so both + # survive — the group value under ``group.count`` and the metric under + # ``aggregates.count``. + ids = sorted(Folder.objects.create(name=n).pk for n in ("a", "b", "c")) + + result = self._tools().list_rows( + "filer_folder", + fields=["id", "name"], + group_by=["count"], + aggregate=[{"fn": "count"}], + ) + + self.assertEqual( + result["groups"], + [{"group": {"count": pk}, "aggregates": {"count": 1}} for pk in ids], + ) + def test_aggregate_without_group_by_still_returns_scalar_dict(self): # Back-compat: the degenerate single-constant-group path keeps the # original scalar shape under "aggregates". diff --git a/src/django_smartbase_admin/mcp/tests/test_oauth_redirect_loopback.py b/src/django_smartbase_admin/mcp/tests/test_oauth_redirect_loopback.py index 061aa954..9e654044 100644 --- a/src/django_smartbase_admin/mcp/tests/test_oauth_redirect_loopback.py +++ b/src/django_smartbase_admin/mcp/tests/test_oauth_redirect_loopback.py @@ -81,6 +81,23 @@ def test_validator_ignores_port_for_loopback_redirects(self): loopback_redirect_allowed("http://evil.example:52756/callback", registered) ) + def test_loopback_fallback_is_http_only(self): + """Port relaxation must not smuggle a non-http scheme past the allowlist. + + DOT rejects a scheme outside ``ALLOWED_REDIRECT_URI_SCHEMES``; the + loopback fallback must not re-admit it just because a registered URI + shares the (non-http) scheme, loopback host, path and query but a + different port. + """ + registered = ["javascript://localhost:1/callback"] + self.assertFalse( + loopback_redirect_allowed("javascript://localhost:9/callback", registered) + ) + # Even an exact non-http loopback match goes through DOT, never here. + self.assertFalse( + loopback_redirect_allowed("javascript://localhost:1/callback", registered) + ) + # End-to-end through the validator: DOT's exact match fails on the # port, the loopback fallback recovers it; no client -> no fallback. validator = SBAdminMCPOAuth2Validator()