gabotorresruiz commented on PR #43974:
URL: https://github.com/apache/superset/pull/43974#issuecomment-5589926371
Hi @Ujjwaljain16, thanks for the careful work here, and especially for
digging into the three URI forms yourself. Good catch on the rename hazard with
`database_name` too.
I verified the mechanics on this branch independently:
`make_url("mssql+pyodbc://SuperSet:pw@abcm")` yields `database=None` and this
branch's `raise_for_access` still denies the issue's repro; the `odbc_connect`
style yields `database=''` (empty string, not None) and is denied as well; the
path form is authorized correctly, and that is the form in `MssqlEngineSpec`'s
own suggested connection string, so the fix as written has real value. 3 of the
7 new tests fail on master, so the regression guards are genuine.
On your scoping question: yes, I would be comfortable with an engine-spec
hook that resolves the database only when it is statically present in the
connection config, leaving the host/DSN-only case fail-closed and documented.
There is direct precedent in `get_schema_from_engine_params`, and I agree live
resolution of the login's default database has no place in the authorization
path. Three asks to go with it: handle the `odbc_connect` case explicitly
(empty string, not None); build at least one test from a real URI through
`make_url` instead of hardcoding `url_object.database` on the mock, per
Renata's review; and change "Fixes #31406" to "partially addresses", noting the
limitation on the issue so it does not auto-close with the reporter's config
still broken.
I also agree with keeping the comparison exact rather than case-folding: a
false negative falls back to the pre-existing denial, while a wrong case-fold
would teach the check to merge two distinct databases. Happy to take another
look once the hook version is up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
