Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use of Django connection name rather than alias as lookup #456

Open
sdether opened this issue Aug 22, 2021 · 3 comments
Open

Use of Django connection name rather than alias as lookup #456

sdether opened this issue Aug 22, 2021 · 3 comments

Comments

@sdether
Copy link

sdether commented Aug 22, 2021

While looking through the connection logic for my attempt to address #455, I noticed that the name used to get a connection is the django connection name rather than the alias. The implications of this are:

  • security
    • if you know a django connection name you can access it even if explorer is not mapped to allow access to it, since EXPLORER_CONNECTIONS is only used to build the intended connections, not to sanity check the provided one
  • usability
    • you cannot cahnge the targeted django connection of a stored Query
      • if you realize that you want to create a new connection with different credentials, only new Query instances will store that connection and all old ones continue using the existing one
      • if you need to rename a django connection, all existing Query instances will be broken

It would be better if the stored connection was the explorer's name rather than the django name, so that remapping is possible and EXPLORER_CONNECTIONS can be used to gate access to the defined database connections.

If this change in behavior is acceptable, I could prepare a PR to implement it, including a migration that re-writes the stored Query.connection values.
@marksweb
Copy link
Collaborator

@sdether Thanks for raising. Could you link to the relevant parts of the code please? Makes it a little easier for anyone reading the issue to see what you're referring to. Especially for me, because although I maintain the package, I didn't write it.

I think this may tie in with #453 which talked about issues when using different connections.

What you're saying certainly makes sense. So happy for changes to happen.

@sdether
Copy link
Author

sdether commented Aug 24, 2021 •
edited by marksweb
Loading

Yeah, I believe that #453 is affected by this. There isn't a clear place to point at in code other than pointing out that the conncections dictionary is just a proxy to the django connections dictionary, i.e. implicitly requires django connection names, not the defined aliases:

https://github.com/groveco/django-sql-explorer/blob/499f18d0efee8b0d7e739b764a52b9ba758b6d89/explorer/connections.py#L21

I'll create a PR which will more clearly illustrate and fix this.

@marksweb
Copy link
Collaborator

Thanks @sdether I had a look around the use of connections as well & this looked like it'd come under this PR as it's taking that alias into account if the Query has that connection field utilised.

https://github.com/groveco/django-sql-explorer/blob/499f18d0efee8b0d7e739b764a52b9ba758b6d89/explorer/utils.py#L161

sdether added a commit to sdether/django-sql-explorer that referenced this issue Aug 27, 2021
@sdether
Issue explorerhq#456: Use of Django connection name rather than alias…
9c54920 
… as lookup

This commit changes connection behavior from storing the database
connection name to using the database alias mapped by SQL Explorer
instead.

The reason for this change is two-fold:
1) Views take the connection name as input, allowing anyone who knows
   Django connection names to query those databases, even if SQL
   does not expose the connection directly.
2) `Query` stores the connection name, which means that if the
   Django connection name changes or a different connection should
   be used (for example, one with reduced permissions) the stored
   Query will either stop working or at least continue using the old
   connection

This change modifies `ExplorerConnections` from being a dictionary
that proxies the Django connection dictionary to a dictionary-like
object that uses `EXPLORER_CONNECTIONS` to lookup and validate
the requested connection alias.

In addition all code that used to the `EXPLORER_CONNECTIONS` value
now uses the key instead.

For backwards compatibility, a migration will back-populate the alias
into `Query` instances (and fail if the mapping no longer exists),
`EXPLORER_DEFAULT_CONNECTION` is re-written on start-up to use the
alias in case it still uses the Django Connection name and
`ExplorerConnections` will still accept a Django Connection name as
long as that name is exposed by some alias in `EXPLORER_CONNECTIONS`.
sdether added a commit to sdether/django-sql-explorer that referenced this issue Aug 31, 2021
@sdether
Issue explorerhq#456: Use of Django connection name rather than alias…
93c4298 
… as lookup

This commit changes connection behavior from storing the database
connection name to using the database alias mapped by SQL Explorer
instead.

The reason for this change is two-fold:
1) Views take the connection name as input, allowing anyone who knows
   Django connection names to query those databases, even if SQL
   does not expose the connection directly.
2) `Query` stores the connection name, which means that if the
   Django connection name changes or a different connection should
   be used (for example, one with reduced permissions) the stored
   Query will either stop working or at least continue using the old
   connection

This change modifies `ExplorerConnections` from being a dictionary
that proxies the Django connection dictionary to a dictionary-like
object that uses `EXPLORER_CONNECTIONS` to lookup and validate
the requested connection alias.

In addition all code that used to the `EXPLORER_CONNECTIONS` value
now uses the key instead.

For backwards compatibility, a migration will back-populate the alias
into `Query` instances (and fail if the mapping no longer exists),
`EXPLORER_DEFAULT_CONNECTION` is re-written on start-up to use the
alias in case it still uses the Django Connection name and
`ExplorerConnections` will still accept a Django Connection name as
long as that name is exposed by some alias in `EXPLORER_CONNECTIONS`.
sdether added a commit to sdether/django-sql-explorer that referenced this issue Aug 31, 2021
@sdether
Issue explorerhq#456: Use of Django connection name rather than alias…
ecb960d 
… as lookup

This commit changes connection behavior from storing the database
connection name to using the database alias mapped by SQL Explorer
instead.

The reason for this change is two-fold:
1) Views take the connection name as input, allowing anyone who knows
   Django connection names to query those databases, even if SQL
   does not expose the connection directly.
2) `Query` stores the connection name, which means that if the
   Django connection name changes or a different connection should
   be used (for example, one with reduced permissions) the stored
   Query will either stop working or at least continue using the old
   connection

This change modifies `ExplorerConnections` from being a dictionary
that proxies the Django connection dictionary to a dictionary-like
object that uses `EXPLORER_CONNECTIONS` to lookup and validate
the requested connection alias.

In addition all code that used to the `EXPLORER_CONNECTIONS` value
now uses the key instead.

For backwards compatibility, a migration will back-populate the alias
into `Query` instances (and fail if the mapping no longer exists),
`EXPLORER_DEFAULT_CONNECTION` is re-written on start-up to use the
alias in case it still uses the Django Connection name and
`ExplorerConnections` will still accept a Django Connection name as
long as that name is exposed by some alias in `EXPLORER_CONNECTIONS`.
sdether added a commit to sdether/django-sql-explorer that referenced this issue Aug 31, 2021
@sdether
Issue explorerhq#456: Use of Django connection name rather than alias…
0bd3f1b 
… as lookup

This commit changes connection behavior from storing the database
connection name to using the database alias mapped by SQL Explorer
instead.

The reason for this change is two-fold:
1) Views take the connection name as input, allowing anyone who knows
   Django connection names to query those databases, even if SQL
   does not expose the connection directly.
2) `Query` stores the connection name, which means that if the
   Django connection name changes or a different connection should
   be used (for example, one with reduced permissions) the stored
   Query will either stop working or at least continue using the old
   connection

This change modifies `ExplorerConnections` from being a dictionary
that proxies the Django connection dictionary to a dictionary-like
object that uses `EXPLORER_CONNECTIONS` to lookup and validate
the requested connection alias.

In addition all code that used to the `EXPLORER_CONNECTIONS` value
now uses the key instead.

For backwards compatibility, a migration will back-populate the alias
into `Query` instances (and fail if the mapping no longer exists),
`EXPLORER_DEFAULT_CONNECTION` is re-written on start-up to use the
alias in case it still uses the Django Connection name and
`ExplorerConnections` will still accept a Django Connection name as
long as that name is exposed by some alias in `EXPLORER_CONNECTIONS`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@sdether @marksweb