fix: default RESOURCE_SERVER__URL to None to restore standalone auth#1503
fix: default RESOURCE_SERVER__URL to None to restore standalone auth#1503
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1503 +/- ##
==========================================
- Coverage 91.83% 91.82% -0.01%
==========================================
Files 237 237
Lines 10482 10482
==========================================
- Hits 9626 9625 -1
- Misses 856 857 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
|
/run-e2e |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughModifies the default value of the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can get early access to new features in CodeRabbit.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/aap_eda/settings/defaults.py`:
- Line 196: The signal handler gw_handler and the event_stream API view
instantiate SyncCertificates without guarding against
settings.RESOURCE_SERVER["URL"] being None; either add early guards in those
call sites to skip creating SyncCertificates when
settings.RESOURCE_SERVER.get("URL") is falsy (i.e., return/skip sync) or add a
defensive check in SyncCertificates.__init__ that validates
settings.RESOURCE_SERVER.get("URL") and raises a clear ValueError. Update
gw_handler (sync_certs.py) and the event_stream view (event_stream.py) to check
settings.RESOURCE_SERVER.get("URL") before calling SyncCertificates, or
implement the check inside SyncCertificates.__init__ (and adjust call sites to
handle the exception) so urljoin(self.gateway_url, ...) cannot receive None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61ba66c6-80d4-4cbc-9446-7a7bbf9831b1
📒 Files selected for processing (1)
src/aap_eda/settings/defaults.py
| # -------------------------------------------------------- | ||
|
|
||
| RESOURCE_SERVER__URL: Optional[str] = "https://localhost" | ||
| RESOURCE_SERVER__URL: Optional[str] = None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SyncCerts implementation (URL usage) =="
fd 'sync_certs.py$' --exec sed -n '1,260p' {}
echo
echo "== SyncCerts instantiations with context =="
rg -nP --type=py -C4 '\bSyncCerts\s*\('
echo
echo "== RESOURCE_SERVER URL guards with context =="
rg -nP --type=py -C3 'RESOURCE_SERVER\["URL"\]'
echo
echo "== URL composition sites that can fail on None =="
rg -nP --type=py -C3 'urljoin\s*\(\s*self\.gateway_url\s*,|f"\{settings\.RESOURCE_SERVER\["URL"\]'Repository: ansible/eda-server
Length of output: 18410
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Finding gw_handler signal registration =="
rg -nP --type=py 'gw_handler|post_save.*EdaCredential' -B2 -A2
echo
echo "== All SyncCertificates instantiations in production code =="
rg -nP --type=py 'SyncCertificates\s*\(' --glob='!test*.py' --glob='!*/tests/*' -B3 -A1
echo
echo "== Checking RESOURCE_SERVER["URL"] guards in production =="
rg -nP --type=py 'if.*RESOURCE_SERVER\["URL"\]|RESOURCE_SERVER\["URL"\]\s*and' --glob='!test*.py' --glob='!*/tests/*' -B2 -A2Repository: ansible/eda-server
Length of output: 7725
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full gw_handler implementation =="
sed -n '286,320p' src/aap_eda/services/sync_certs.py
echo
echo "== event_stream.py instantiation context =="
sed -n '355,375p' src/aap_eda/api/views/event_stream.py
echo
echo "== Checking if RESOURCE_SERVER["URL"] is checked anywhere before signal handler calls =="
rg -nP --type=py 'if.*RESOURCE_SERVER' --glob='!test*.py' --glob='!*/tests/*' -B1 -A1Repository: ansible/eda-server
Length of output: 2618
Both SyncCertificates call paths lack guards for None URL and will fail at runtime
With RESOURCE_SERVER__URL now defaulting to None, the signal handler in gw_handler() (line 302 of sync_certs.py) and the API view in event_stream.py (line 363) will both instantiate SyncCertificates without checking whether settings.RESOURCE_SERVER["URL"] is set. Line 42 assigns directly: self.gateway_url: str = settings.RESOURCE_SERVER["URL"], and later calls like urljoin(self.gateway_url, slug) (lines 125, 133, 227, 241) will raise TypeError when gateway_url is None.
Add a guard in both call sites to skip sync when settings.RESOURCE_SERVER.get("URL") is falsy, or add a check in SyncCertificates.__init__ to raise a clearer error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/aap_eda/settings/defaults.py` at line 196, The signal handler gw_handler
and the event_stream API view instantiate SyncCertificates without guarding
against settings.RESOURCE_SERVER["URL"] being None; either add early guards in
those call sites to skip creating SyncCertificates when
settings.RESOURCE_SERVER.get("URL") is falsy (i.e., return/skip sync) or add a
defensive check in SyncCertificates.__init__ that validates
settings.RESOURCE_SERVER.get("URL") and raises a clear ValueError. Update
gw_handler (sync_certs.py) and the event_stream view (event_stream.py) to check
settings.RESOURCE_SERVER.get("URL") before calling SyncCertificates, or
implement the check inside SyncCertificates.__init__ (and adjust call sites to
handle the exception) so urljoin(self.gateway_url, ...) cannot receive None.
After we merged this [1], all user-facing API access to EDA must goes through the AAP gateway when deployed as part of the platform. However, RESOURCE_SERVER__URL defaults to "https://localhost", which causes the JWT-only authentication override in default.py to fire unconditionally. This breaks session-based UI login for standalone deployments. Setting the default to None ensures SessionAuthentication is preserved unless RESOURCE_SERVER__URL is explicitly configured, which only happens when deployed behind the AAP Gateway. The reason this didn't break any development scenarios, "task docker:up" or similar is because docker-compose-dev.yaml sets EDA_MODE=development, which loads up development_defaults.py instead, which then sets RESOURCE_SERVER["URL"] to None by default. [1] #1495
ptoscano
force-pushed
the
fix-default-resource-server-url
branch
from
7954689 to
ef0c29d
Compare
|
|
I'm closing this PR in favor of ansible/eda-server-operator#331 |
Pull request was closed
4 participants
After we merged this [1], all user-facing API access to EDA must go
through the AAP gateway when deployed as part of the platform.
However,
RESOURCE_SERVER__URLdefaults to"https://localhost", whichcauses the JWT-only authentication override in default.py to fire unconditionally.
This breaks session-based UI login for standalone deployments.
Setting the default to None ensures SessionAuthentication is preserved unless
RESOURCE_SERVER__URLis explicitly configured, which only happens whendeployed behind the AAP Gateway.
The reason this didn't break any development scenarios,
"task docker:up"or similaris because docker-compose-dev.yaml sets
EDA_MODE=development, which loadsup development_defaults.py instead, which then sets
RESOURCE_SERVER["URL"]toNone by default.
[1] #1495
Summary by CodeRabbit