From b2f4cffc215e39cb2ee879de0ad969b0c3a6f036 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Percy=20Camilo=20Trive=C3=B1o=20Aucahuasi?= Date: Thu, 9 Jul 2026 21:31:16 -0500 Subject: [PATCH] fix(auth): report rejected org switches and tolerate a null active_organization Two defects on the login path, both of which turn a server response the client should act on into silence. 1. _switch_org posted to /api/v2/o//switch/, handed the response to log_requests_error (which only logs and never raises), then cached the (org, token) pair unconditionally. A server that rejected the switch was recorded as having accepted it, and the early return at the top of the method made every later attempt a silent no-op for the rest of the session. The client could not distinguish a 200 from a 403. The pair is now cached only on a 2xx response. The method returns a bool so callers can tell acceptance from rejection, and logs a warning naming the status code. A rejected switch is retried on the next call. Login still succeeds when the switch fails, because not every deployment exposes the endpoint, so callers must not treat False as fatal. 2. _finalize_login read json_response.get('active_organization', {}) and then called .get('slug') on the result. The dict default only covers a missing key, so a server sending "active_organization": null, or any non-dict value, raised AttributeError rather than reading as "no organization bound". The username/password and personal-key paths now apply the same isinstance guard the SSO path already used. Tests: nine cases in graphistry/tests/test_arrow_uploader.py covering accepted, rejected, retried, transport-failed, already-cached and no-op switches, plus login against a null, missing and non-dict active_organization. Existing coverage mocked _switch_org itself, so it asserted only that the client calls switch, never that the server accepted it. All nine fail on the prior implementation. --- CHANGELOG.md | 2 + graphistry/arrow_uploader.py | 44 ++++++--- graphistry/tests/test_arrow_uploader.py | 113 ++++++++++++++++++++++++ 3 files changed, 149 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f844842c8..26db470125 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - **GFQL Cypher parser: Earley → LALR(1) (~70× faster parse, same AST)**: `parse_cypher` now uses a single LALR(1) parser (contextual lexer) instead of Earley (dynamic lexer) — ~0.25ms vs ~17ms per query on representative queries. The WHERE grammar was unified to one `where_clause: "WHERE" expr` rule (the former `where_predicates | expr` dual rule was a genuine reduce/reduce ambiguity that forced Earley), and the structured flat-AND predicate form is recovered by a post-parse lift that re-parses the WHERE body with a LALR sub-parser (`start="where_predicate_chain"`). The full WHERE language is preserved — OR/XOR/NOT, parentheses, arithmetic, IN, pattern predicates — nothing is restricted to a subset. The grammar was then **purified to (near-)unambiguous**: WHERE binds to its preceding clause *in the grammar* (bundled into `match_clause`; the WITH..WHERE attachment ambiguity is eliminated, not tie-broken), and name-rooted dot chains derive only via `qualified_name` (the `property_access` redundancy is gone) — dropping the redundant `label_predicate_expr` rule (`(n:Admin)` parses via `grouped_expr`), and excluding a top-level `IN` from list-literal elements (`[x IN xs ...` is comprehension syntax; allowing a bare `IN` list element made it overlap). Net: the LALR conflict profile goes from 8 shift/reduce to **ZERO conflicts** — the grammar is now **provably unambiguous LALR(1)** and builds under Lark's `strict=True` (a build-time proof, machine-checked in CI as zero conflicts, plus a strict-mode build test where the optional `interegular` dep is present). Every input has a single derivation. **Machine-checked invariants** (`test_grammar_invariants.py`): (1) **zero LALR conflicts** (dependency-free, always-on in CI) plus a `strict=True` build test (skipped where the optional `interegular` dep is absent) — a grammar edit that introduces any ambiguity fails CI; (2) semantic ambiguity is ZERO — every corpus query has exactly one Earley derivation and it transforms to a single AST, no exceptions; (3) a rule-coverage gate (`test_every_grammar_rule_is_exercised_by_the_corpus`) forces every new grammar rule through the invariants; (4) differential tests run the production pipeline under both parsers (byte-identical ASTs, identical rejections). **Full-repo differentials** (1,850+ scraped queries, LALR-vs-Earley and old-vs-new production): **zero AST divergences**, and a small set of deliberate language fixes — accept-by-accident shapes with ill-defined semantics, now honest syntax errors and pinned as tests (`DELIBERATE_LANGUAGE_FIXES`): `RETURN DISTINCT` with no items (Earley re-lexed DISTINCT as a variable name), double WHERE (the old positional attachment kept *both* predicates in different AST fields), double post-WITH WHERE, WHERE after UNWIND, WHERE before MATCH in a graph constructor, and the invalid "list of IN-booleans" (`[x IN xs, y]`; use `[(x IN xs), y]` for a genuine bool list). The lift stays an internal optimization: only a flat `AND` chain over present columns lifts to `filter_dict`; parens / OR / XOR / NOT and any absent-column case stay on `where_rows` (a correctness boundary, since `where_rows` treats an absent property as null while `filter_dict` would raise). Full cypher suite (1,681 tests) passes. Downstream execution (`filter_dict` vs `where_rows` routing) is unchanged. A pre-existing `<>`-over-null 3VL divergence between the two execution paths (surfaced by #1653's metamorphic test) is tracked separately in #1683. ### Fixed +- **`_switch_org` no longer treats a rejected organization switch as a success**: `ArrowUploader._switch_org` posted to `/api/v2/o//switch/`, passed the response to `log_requests_error` (which only logs, never raises), and then cached the `(org, token)` pair unconditionally. A server that rejected the switch was therefore recorded as having accepted it, and the early return at the top of the method turned every later attempt into a silent no-op for the rest of the session. The method now records the pair only on a 2xx response, returns a bool so callers can tell acceptance from rejection, and logs a warning naming the status code. A rejected switch is retried on the next call rather than skipped. Login still succeeds when the switch fails, since not every deployment exposes the endpoint. +- **Login no longer crashes when the server sends a null `active_organization`**: `_finalize_login` read `json_response.get('active_organization', {})` and then called `.get('slug')` on the result. The dict default only covers a missing key, so a server sending `"active_organization": null` (or any non-dict) raised `AttributeError` instead of being read as "no organization bound". The username/password and personal-key login paths now apply the same `isinstance` guard the SSO path already used. - **GFQL Cypher `GRAPH { }` residual predicates now fail safely or apply as graph masks**: `GRAPH { MATCH ... WHERE ... }` no longer silently drops predicates that the graph-state path cannot apply. Safe one-node/one-edge residual filters, including disjunctions and `searchAny(...)`, are applied before graph-state matching; unsupported pattern-predicate, multi-alias, and Polars graph-residual cases now raise clear validation errors instead of returning an over-broad subgraph. - **GFQL Cypher `=~` / scalar-fn cross-engine hardening (review wave, dgx-verified)**: (1) composed `=~` (`WHERE … =~ … OR …`, `RETURN`-expression position) now works on `engine='cudf'` — the series evaluator used raw `.str.fullmatch`, which cuDF lacks, and the resulting `AttributeError` was masked as "unsupported predicate op" (it now routes through the `Fullmatch` predicate's engine workarounds, and honest `NotImplementedError` declines pass through instead of being re-labeled); (2) the cuDF fullmatch emulation anchors alternations as a whole (`^(ab|cd)$` — bare `^ab|cd$` silently matched `'abXXX'`); (3) the cuDF case-insensitive `(?i)` lowercase-folding workaround now declines the fold-unsound shapes — uppercase escape classes (`.lower()` turns `\D` into `\d`, silently inverting the predicate), case-crossing character ranges (`(?i)[A-z]` silently narrowed; `[X-b]` folded to an invalid range), and non-ASCII patterns — while lowercase escapes (`\d`, `\.`) keep folding as before; lookaround, backreferences, and named-group refs decline up front (libcudf rejects them at kernel-compile time); (4) polars `Match`/`Fullmatch` lowering applies the same Rust-regex guard as `Contains` (lookaround/backrefs decline instead of a non-NIE `ComputeError` at collect); (5) `toLower`/`toUpper` on a non-string column decline like neo4j's type error instead of broadcasting the stringified Series repr (pandas/cuDF) or raising a non-NIE `SchemaError` (polars-gpu); (6) polars `floor`/`ceil`/`round` cast to `Float64` so integer columns return Float like neo4j and the pandas engine; (7) invalid regex patterns on the composed path raise a clear "invalid regex pattern" error instead of "unsupported predicate op". - **GFQL Cypher `round()` hardening (review wave, dgx-verified)**: (1) the `polars` extra's floor is now `polars>=1.29` — `Expr.round(mode=)` shipped in py-1.29.0 (pola-rs/polars#22248), not 1.5 as previously pinned, so 1.5–1.28 installs crashed with a raw `TypeError` on `round(x, p>0)` under `engine='polars'`; (2) `round(x, p>308)` is the identity on both engines (a float64 has no digits there) instead of pandas raising through an unclear decline while polars returned identity — parity restored, `10.0**p` overflow guarded; (3) polars `round(x, p>0)` normalizes `-0.0` to `+0.0` like the pandas kernel (`round(-0.04, 1)` was `0.0` on pandas vs `-0.0` on polars — invisible to value equality, pinned by a sign-bit test); (4) documented the precision>0 decimal-string deviation vs neo4j (`round(2.675, 2)` = `2.67` binary-double here vs `2.68` BigDecimal there) and added deterministic tie/hazard matrix cases so ties actually reach cuDF/polars-gpu (the fixture's normal-distribution floats never tied). diff --git a/graphistry/arrow_uploader.py b/graphistry/arrow_uploader.py index 91cb6b5f63..e08b9ac1e4 100644 --- a/graphistry/arrow_uploader.py +++ b/graphistry/arrow_uploader.py @@ -266,27 +266,46 @@ def certificate_validation(self, certificate_validation): ########################################################################3 - def _switch_org(self, org_name: Optional[str], token: Optional[str]) -> None: + def _switch_org(self, org_name: Optional[str], token: Optional[str]) -> bool: + """ + Ask the server to make org_name the session's active organization. + + Returns True when the server accepted the switch, False otherwise. A rejected + or failed switch is not recorded, so a later call retries instead of silently + skipping. Callers must not treat False as fatal: not every deployment exposes + the switch endpoint, and login should still succeed. + """ if not org_name or not token: - return + return False last = self._client_session._last_switched_org_token if last == (org_name, token): - return + return True + + switch_url = f"{self.server_base_path}/api/v2/o/{org_name}/switch/" try: - switch_url = f"{self.server_base_path}/api/v2/o/{org_name}/switch/" response = requests.post( switch_url, data={'slug': org_name}, headers=inject_trace_headers({'Authorization': f'Bearer {token}'}), verify=self.certificate_validation, ) - log_requests_error(response) - self._client_session._last_switched_org_token = (org_name, token) - from .pygraphistry import PyGraphistry - if PyGraphistry.session is self._client_session: - PyGraphistry.session._last_switched_org_token = (org_name, token) except Exception as exc: logger.warning("Failed to switch organization %s: %s", org_name, exc) + return False + + log_requests_error(response) + if not (200 <= response.status_code < 300): + logger.warning( + "Server rejected switch to organization %s with HTTP %s; " + "the session may remain bound to a different organization", + org_name, response.status_code) + return False + + self._client_session._last_switched_org_token = (org_name, token) + from .pygraphistry import PyGraphistry + if PyGraphistry.session is self._client_session: + PyGraphistry.session._last_switched_org_token = (org_name, token) + return True def login(self, username, password, org_name=None): @@ -338,7 +357,12 @@ def _finalize_login(self, out: requests.Response, org_name: Optional[str]) -> 'A f"missing 'token' key in response. Response content: {out.text}" ) - org = json_response.get('active_organization',{}) + # A server may omit active_organization, send null, or send a non-dict. + # All three mean "no organization bound". The dict default alone does not + # cover a present-but-null key, which would raise AttributeError below. + org = json_response.get('active_organization', None) + if not isinstance(org, dict): + org = {} logged_in_org_name = org.get('slug', None) if org_name: # caller pass in org_name if not logged_in_org_name: # no active_organization in JWT payload diff --git a/graphistry/tests/test_arrow_uploader.py b/graphistry/tests/test_arrow_uploader.py index 867ea66024..f34ab4274d 100644 --- a/graphistry/tests/test_arrow_uploader.py +++ b/graphistry/tests/test_arrow_uploader.py @@ -809,3 +809,116 @@ def test_payload_length_multiple_of_4_after_rstrip_decodes_correctly(self): token = _make_test_jwt(payload_dict) # The exact byte length will vary; the corrected formula is robust to all. assert _personal_org_from_jwt(token) == 'u' + + +class TestArrowUploader_SwitchOrg(unittest.TestCase): + """ + _switch_org must report whether the server accepted the switch, and must not + remember a switch that never happened. Earlier revisions logged the failure and + then cached the (org, token) pair anyway, so the early return turned every later + attempt into a silent no-op. + """ + + def _mock_response(self, status=200, json_data=None): + mock_resp = mock.Mock() + mock_resp.status_code = status + mock_resp.content = "CONTENT" + mock_resp.raise_for_status = mock.Mock() + mock_resp.json = mock.Mock(return_value=json_data if json_data else {}) + return mock_resp + + def _uploader(self): + au = ArrowUploader() + au._client_session._last_switched_org_token = None + return au + + @mock.patch('requests.post') + def test_switch_org_accepted_is_reported_and_cached(self, mock_post): + mock_post.return_value = self._mock_response(status=200) + + au = self._uploader() + assert au._switch_org("my-org", "tok") is True + assert au._client_session._last_switched_org_token == ("my-org", "tok") + + @mock.patch('requests.post') + def test_switch_org_rejected_is_reported_and_not_cached(self, mock_post): + mock_post.return_value = self._mock_response(status=403) + + au = self._uploader() + assert au._switch_org("my-org", "tok") is False + assert au._client_session._last_switched_org_token is None + + @mock.patch('requests.post') + def test_switch_org_rejection_does_not_suppress_a_retry(self, mock_post): + mock_post.return_value = self._mock_response(status=403) + au = self._uploader() + assert au._switch_org("my-org", "tok") is False + + mock_post.return_value = self._mock_response(status=200) + assert au._switch_org("my-org", "tok") is True + assert mock_post.call_count == 2 + + @mock.patch('requests.post') + def test_switch_org_transport_failure_is_reported_and_not_cached(self, mock_post): + mock_post.side_effect = Exception("connection reset") + + au = self._uploader() + assert au._switch_org("my-org", "tok") is False + assert au._client_session._last_switched_org_token is None + + @mock.patch('requests.post') + def test_switch_org_skips_http_when_already_switched(self, mock_post): + au = self._uploader() + au._client_session._last_switched_org_token = ("my-org", "tok") + + assert au._switch_org("my-org", "tok") is True + mock_post.assert_not_called() + + @mock.patch('requests.post') + def test_switch_org_without_org_or_token_is_a_noop(self, mock_post): + au = self._uploader() + assert au._switch_org(None, "tok") is False + assert au._switch_org("my-org", None) is False + mock_post.assert_not_called() + + +class TestArrowUploader_NullActiveOrganization(unittest.TestCase): + """ + A server may omit active_organization, send null, or send a non-dict. The dict + default on .get() only covers a missing key, so a present-but-null value used to + raise AttributeError on the login path. + """ + + def _mock_response(self, json_data): + mock_resp = mock.Mock() + mock_resp.status_code = 200 + mock_resp.content = "CONTENT" + mock_resp.raise_for_status = mock.Mock() + mock_resp.json = mock.Mock(return_value=json_data) + return mock_resp + + @mock.patch('requests.post') + def test_login_with_null_active_organization(self, mock_post): + mock_post.return_value = self._mock_response( + {'token': '123', 'active_organization': None}) + + au = ArrowUploader() + assert au.login(username="u", password="p").token == "123" + assert PyGraphistry.org_name() is None + + @mock.patch('requests.post') + def test_login_with_missing_active_organization(self, mock_post): + mock_post.return_value = self._mock_response({'token': '123'}) + + au = ArrowUploader() + assert au.login(username="u", password="p").token == "123" + assert PyGraphistry.org_name() is None + + @mock.patch('requests.post') + def test_login_with_non_dict_active_organization(self, mock_post): + mock_post.return_value = self._mock_response( + {'token': '123', 'active_organization': 'unexpected-string-shape'}) + + au = ArrowUploader() + assert au.login(username="u", password="p").token == "123" + assert PyGraphistry.org_name() is None