galaxyproject / gxadmin

Handy command line utility for Galaxy administrators :rocket:
https://galaxyproject.github.io/gxadmin/#/
GNU General Public License v3.0
23 stars 27 forks source link

New params in TPT query to indicate #137

Open hujambo-dunia opened 1 year ago

hujambo-dunia commented 1 year ago

Few questions for us to ponder:

  1. (high) What is else is necessary to prevent SQL injection?
  2. (moderate) More elegant way to pull in variables: arrSelect1 and arrWhere1 (JSON, multidimensional array, etc)?

hexylena commented 1 year ago

SQL Injection

This is not a priority. This project will probably never add anything to prevent SQL injection outside of common sense checks on input. See e.g. q and aq in gxadmin query. gxadmin simply passes sql to the psql client, if you can run gxadmin, you can already also run arbitrary queries with psql. I do not think it is technically possible to provide any meaningful security layer between the user and psql.

Admins installing gxadmin should understand that they're granting read write database access to anyone with gxadmin access. You could setup a separate role that e.g. restricts table access. If SQL injection is a concern, then I strongly recommend doing this. A read-only DB account will prevent a lot of issues.

I think EU is currently the only place granting a select group of users the ability to run gxadmin just for statistics queries, but there I believe they use read-only access (right @mira-miracoli ?)

More elegant way to pull in variables: arrSelect1 and arrWhere1 (JSON, multidimensional array, etc)?

I'm not sure there is a more elegant way, but the current changes are hmm. It's not my query so I won't push back too hard, but, providing extra bits of sql in this way is not a very user friendly or ergonomic design.

You're also hitting the limits of our argument parsing library there. The ##? indicates that everything that follows should be parsed by our argument parser. If you do not explicitly name the additional arguments, they will not be set as parameters for the function: an error should be thrown.

We only have one other function that accepts arbitrary numbers of arguments but it's function signature is much simpler.

I'd honestly suggest maybe separate queries per metric, or a single query with an optional --destination filter for the stampede cluster, and another optional --metric that lets you override the default metric.

For design inspiration you could look at the jobs query which lets you filter by destination/user/a few other things. But they're all explicitly named arguments (which generally i think is easier for users because they'll know what's available, rather than having to dig into the DB to figure out which columns they can filter against).

kysrpex commented 1 year ago

@hexylena Yes, in EU a select group of users can log-in as a specific Linux user that has read-only access to the database, so their gxadmin calls cannot alter the database contents.