Skip to content

Commit 9c846fd

Browse files
fix: implement CodeRabbit suggestions
- Gate cancellation UX with use_redirect_auth, not return_url. - Add a popup-mode regression test when return_url is present. Signed-off-by: Patrick Chin <8509935+thepatrickchin@users.noreply.github.com>
1 parent a0e1bb0 commit 9c846fd

3 files changed

Lines changed: 14 additions & 2 deletions

File tree

packages/nvidia_nat_core/src/nat/front_ends/fastapi/routes/auth.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ async def redirect_uri(request: Request):
6262
if not flow_state.future.done():
6363
flow_state.future.set_exception(
6464
RuntimeError(f"Authorisation denied: {error} ({error_description})"))
65-
if flow_state.return_url:
65+
if flow_state.config and flow_state.config.use_redirect_auth:
6666
return HTMLResponse(content=build_auth_redirect_cancelled_html(flow_state.return_url),
6767
status_code=200,
6868
headers={

packages/nvidia_nat_core/tests/nat/front_ends/auth_flow_handlers/test_websocket_flow_handler.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ async def create_websocket_message(self, msg):
174174
token_val = ctx.headers["Authorization"].split()[1]
175175
assert token_val in mock_server.tokens, "token not issued by mock server"
176176

177-
# all flowstate cleaned up
177+
# all flow-state cleaned up
178178
assert worker._outstanding_flows == {}
179179

180180

packages/nvidia_nat_core/tests/nat/front_ends/fastapi/test_auth_redirect_route.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,18 @@ async def test_access_denied_popup_returns_cancelled_html():
8484
assert "AUTH_ERROR" not in response.text
8585

8686

87+
async def test_access_denied_popup_with_return_url_still_returns_cancelled_html():
88+
"""error=access_denied in popup mode uses popup HTML even when return_url is set."""
89+
flow_state = FlowState(config=_POPUP_CONFIG, return_url=_RETURN_URL)
90+
worker = _make_worker(flow_state)
91+
response = await _get(worker, {"state": "teststate", "error": "access_denied"})
92+
assert response.status_code == 200
93+
assert "AUTH_CANCELLED" in response.text
94+
assert "AUTH_ERROR" not in response.text
95+
assert _RETURN_URL.replace("/", "\\u002f") not in response.text
96+
assert "oauth_auth_completed" not in response.text
97+
98+
8799
async def test_access_denied_redirect_returns_cancelled_html():
88100
"""error=access_denied in redirect mode returns the redirect-back cancelled page."""
89101
flow_state = FlowState(config=_REDIRECT_CONFIG, return_url=_RETURN_URL)

0 commit comments

Comments
 (0)