django-sql-explorer icon indicating copy to clipboard operation
django-sql-explorer copied to clipboard

Use of Django connection name rather than alias as lookup

Open sdether opened this issue 4 years ago • 3 comments

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.

sdether avatar Aug 22 '21 00:08 sdether

@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.

marksweb avatar Aug 22 '21 21:08 marksweb

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.

sdether avatar Aug 24 '21 18:08 sdether

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

marksweb avatar Aug 24 '21 20:08 marksweb