Ujjwaljain16 commented on PR #43974: URL: https://github.com/apache/superset/pull/43974#issuecomment-5587142721
@rebenitez1802 Thanks for the review i dug a bit deeper into the URI/database identity issue and i think i found an important constraint before i change the implementation for the exact URI from the issue: `mssql+pyodbc://SuperSet:***@abcm` SQLAlchemy parses `abcm` as the **host**, with `url_object.database = None` i also checked the other MSSQL connection styles Superset supports there seem to be three cases: * `mssql+pymssql://.../abcm` → the database is available from the URL * `mssql+pyodbc:///?odbc_connect=...;Database=abcm;...` → the database is available inside the ODBC connection string. * `mssql+pyodbc://SuperSet:***@abcm` → there isn't actually any database information in the URI/config. `abcm` is the host/DSN, and the real database comes from the SQL Server login's default database i also looked at using `database.database_name` as a fallback i don't think that's safe: Superset treats that as the name/identifier of the Superset connection and it can be renamed independently of the underlying database using it as an authorization identity could therefore potentially turn a previously denied cross-database reference into an allowed one the cleanest approach i found is to add a small engine-spec hook, similar to the existing `get_schema_from_engine_params()`, which resolves the database only when it's actually present in the connection configuration (URL database, `connect_args`, or `Database=` in `odbc_connect`). `raise_for_access()` can then normalize the catalog only when it matches that statically-known database. The catch is that this **doesn't make the exact host/DSN-only URI from the issue work**. To safely determine its actual database we'd need to connect to SQL Server and resolve the login's default database, which would introduce live I/O into the authorization path. before i implement this would you be comfortable with the PR being scoped to the statically-resolvable MSSQL cases, while leaving the host/DSN-only case fail-closed and documenting it as a limitation? If the expectation is that the literal URI in #31406 must also work i think we should discuss the live-resolution approach separately first, rather than introducing a heuristic that could affect authorization. -- 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]
