Follow-up from #46.
Context
#46 extracted SDWANManagerAuth.get_session_auth(cls, url, username, password) as a public entry point from the old parameterless _get_session_auth(cls). It's inconsistent with the ACI/CC pattern:
| Adapter |
Method |
verify_ssl param? |
| ACI |
APICAuth.get_token(cls, url, username, password, verify_ssl=False) (aci/auth.py:179) |
✅ explicit |
| CC |
CatalystCenterAuth.get_token(cls, url, username, password, verify_ssl=False) |
✅ explicit |
| SDWAN |
SDWANManagerAuth.get_session_auth(cls, url, username, password) (sdwan/auth.py:423) |
❌ resolves internally via should_verify_ssl("SDWAN") at :441 |
In sdwan/api_test_base.py:113, self.verify_ssl is already computed by the caller, but never passed through — the callee re-derives it.
Why this matters (context — see #46 review comment)
The reason this is worth fixing (beyond consistency) is that this exact asymmetry — some paths accepting verify_ssl explicitly, others computing it internally — is the shape of the ACI auth-POST SSL bug flagged in the #46 review as a before-merge item. Making all three adapters symmetric (fully-parameterized verify_ssl) makes that class of "caller thinks the flag is threaded through, callee silently re-derives or defaults" bug structurally impossible in future adapters.
What to do
# sdwan/auth.py
@classmethod
def get_session_auth(
cls, url: str, username: str, password: str, verify_ssl: bool = False,
) -> dict[str, Any]:
url = url.rstrip("/")
...
Then in sdwan/api_test_base.py, pass self.verify_ssl through explicitly, eliminating the redundant second should_verify_ssl("SDWAN") call.
P.S. — This comment was drafted using voice-to-text via Claude Code. If the tone comes across as overly direct or terse, please know that's just how it tends to phrase things. No offense or criticism is intended — this is purely an objective technical review of the PR. Thanks for understanding! 🙂
Follow-up from #46.
Context
#46 extracted
SDWANManagerAuth.get_session_auth(cls, url, username, password)as a public entry point from the old parameterless_get_session_auth(cls). It's inconsistent with the ACI/CC pattern:APICAuth.get_token(cls, url, username, password, verify_ssl=False)(aci/auth.py:179)CatalystCenterAuth.get_token(cls, url, username, password, verify_ssl=False)SDWANManagerAuth.get_session_auth(cls, url, username, password)(sdwan/auth.py:423)should_verify_ssl("SDWAN")at:441In
sdwan/api_test_base.py:113,self.verify_sslis already computed by the caller, but never passed through — the callee re-derives it.Why this matters (context — see #46 review comment)
The reason this is worth fixing (beyond consistency) is that this exact asymmetry — some paths accepting
verify_sslexplicitly, others computing it internally — is the shape of the ACI auth-POST SSL bug flagged in the #46 review as a before-merge item. Making all three adapters symmetric (fully-parameterizedverify_ssl) makes that class of "caller thinks the flag is threaded through, callee silently re-derives or defaults" bug structurally impossible in future adapters.What to do
Then in
sdwan/api_test_base.py, passself.verify_sslthrough explicitly, eliminating the redundant secondshould_verify_ssl("SDWAN")call.P.S. — This comment was drafted using voice-to-text via Claude Code. If the tone comes across as overly direct or terse, please know that's just how it tends to phrase things. No offense or criticism is intended — this is purely an objective technical review of the PR. Thanks for understanding! 🙂