GET and DELETE method for managing api_key

[siddardh-ra]

This PR deals with some updation in the POST method with /api/v1/key?label=label where that label will be associated with the key generated

Added GET /api/v1/key to retrieve the user's keys and DELETE /api/v1/key/{key_id} to delete a key.

Modified the APIKey table with id field as primary_key.

[dbutenhof]

I think we need to cover a few holes, and I've got some comments on consistency, but this is a great start.

[dbutenhof]

pbench.client.IncorrectParameterCount: API template API.KEY requires 1 parameters ({})

Ha. I ran into this myself recently, where a defined URI template parameter is optional. The PbenchServerClient._uri doesn't have any defined way to determine that, and is failing if there are no parameters provided. The easiest way to get around this is to change the client create_api_key method to

    response = self.post(api=API.KEY, {"key": ""}

That'll turn the /key/{key} template into /key/, which ought to work.

It's fairly easy to run the functional tests on your branch before submitting. I always do

jenkins/run tox server python
jenkins/runlocal --cleanup

(Although because I'm on RHEL 8 instead of RHEL 9 or Fedora, I need to prefix the runlocal with BASE_IMAGE=quay.io/centos/centos:stream9 ... but you probably don't!)

[webbnh]

I second Dave's comment: this is a very good start, but it needs some polish. Also, the deletion test raises a concern: I think the ADMIN user should not be receiving FORBIDDEN, so we need to modify that test to use some other user, fix the CUT so that the ADMIN can delete someone else's key, and add a test which proves it.

[dbutenhof]

I'm not happy about the difference between api_key in the table vs key in the .as_json() response. I think we need to fix that. While I'd actually rather rename the column because api_key in the api_keys table seems unnecessary, it'd be easier to just use the "proper" key in .as_json(). 😆

So just a few comments now, and I've closed virtually all of my previous conversations.

[dbutenhof]

One final comment. I didn't make an issue of this during the PR review; but I think I should have, and I'd really like everyone on the team to start thinking of this along with unit testing.

Please consider writing a functional test for this new code. I already created a functional test case to prove that I can create an API key and use it to authenticate a GET /datasets?mine, and it'd be easy to extend that test case (or about as easy to write a second test case) to show GET /key[{key}] as well as DELETE /key/{key}.

You might consider creating a test_api_key.py rather than adding more to test_put.py (which should probably be renamed to test_datasets.py or some such)... aside from the test case I already have, you could write test cases for API Key management that don't require any datasets and therefore don't need to be dependent on any of the existing tests.

But, aside from making the suggestion, I won't require changes for that at this point. (Although if you don't, a follow-up PR with functional tests would be great...)

[webbnh]

This PR is coming along well, but I don't think it's "there" yet.

My foremost concern is that the unit testing seems a little weak (this on top of Dave's excellent point that PR's should include appropriate functional tests along with unit tests). I think that if there were more complete unit tests, some of the items that I'm calling out below might have been self-evident.

• I don't think we have support in this change for Admin users being able to delete other users' keys, and I think we should. This is important functionality, and I think that ignoring it has sent us in an elegant but awkward direction with the currently proposed change.
• I think we need some clarity around the GET /key and GET /key/<id> interfaces. These have a common implementation mostly due to the vagaries of the underlying framework rather than any obvious design intent. I think they need to be teased apart, at least in principle.
• We don't seem to be using the APIKey.query_by_id() function except in the tests (which don't really need it). If that's the case, then please remove it unless there is a compelling reason to retain it (or, at least, move the definition into the test code).
• I don't think the new pbench_user_token() fixture is necessary; we have an existing fixture which should suffice.
• The assertions in the unit tests around the contents of the key seem like they could use some work. In the cases where we check "all" of them, we're not checking all them (e.g., we don't check the creation date and we frequently don't check the user/username); however, in most cases we don't need to check anything other than the ID or value of the key (just enough to ensure that it's the "right" one).
• Also, the tests which check that we can query and fetch keys by some attribute check that we get all the keys which match but that always amounts to "all the keys there are", which suggests that they aren't as rigorous as the should be. (I'll grant, we don't really need for the tests to be checking whether SQLite works, but we do need to be sure that the CUT is using it properly.)
• The testing of GET /key?name seems a little light. I'm not sure that we have any tests which omit the name parameter (we have one test which specifies an empty string, which I don't think amounts to the same thing).

And, I have a few smaller things, below.

[webbnh]

There are also a few lingering items from my earlier review.

• Consider renaming the APIKey column, name, to description (and, Dave's contemplation that we should rename the api_key column to just key 🙂).
• The unit test code is including the key "name" in the API key value when the corresponding code in the server does not.
• The key deletion unit test would be better if there were more than one key in the database when we try to delete the target, because it could then show that the other key is still intact.
• My questions (1, 2) about audit record values.

[webbnh]

I suspect that we don't want (in general) to have a single PR create multiple Alembic migration scripts. Could you merge the two scripts (or regenerate a single one)?

[webbnh]

OK, all the big issues have been addressed, and we're down to just medium, small, and nit-level items...so I'm marking this PR as "approved", and I'll leave it up to you to decide whether to address the remainder.

That said, some of the bigger items remaining are:

• Renaming the name column to description or label.
• Polymorphism issues in the GET method (particularly its docstring)
• Checking in the unit tests for (lack of) collateral affects when deleting keys

[riya-17]

  [2023-05-09 11:28:50] [build-stdout] [INFO] [2] Read 21 nodes and 1 edges from TSG output
  [2023-05-09 11:28:50] [build-stdout] [WARN] [2] Failed to analyse imports of /home/runner/work/pbench/pbench/agent/bench-scripts/test-bin/fio-histo-log-pctiles.py : Syntax Error (line 3)
  [2023-05-09 11:28:50] [build-stdout] [TRACEBACK] [2] "semmle/python/modules.py", line 107, in py_ast
  [2023-05-09 11:28:50] [build-stdout] [TRACEBACK] [2] "semmle/python/modules.py", line 101, in old_py_ast
  [2023-05-09 11:28:50] [build-stdout] [TRACEBACK] [2] "semmle/python/parser/__init__.py", line 100, in parse
  [2023-05-09 11:28:50] [build-stdout] [TRACEBACK] [2] "semmleFile "<string>", line 3
  [2023-05-09 11:28:50] [build-stdout] [TRACEBACK] [2] "semmle/python/imports.py", line 89, in get_import_nodes
  [2023-05-09 11:28:50] [build-stdout] [TRACEBACK] [2] "semmle/python/modules.py", line 119, in py_ast
  [2023-05-09 11:28:50] [build-stdout] [TRACEBACK] [2] "semmle/python/modules.py", line 116, in py_ast
  [2023-05-09 11:28:50] [build-stdout] [TRACEBACK] [2] "semmle/python/parser/tsg_parser.py", line 220, in parse
  [2023-05-09 11:28:50] [build-stdout] [TRACEBACK] [2] "semmleFile "<string>", line 3
  [2023-05-09 11:28:51] [build-stdout] [INFO] [2] Read 21 nodes and 1 edges from TSG output
  [2023-05-09 11:28:51] [build-stdout] [INFO] [2] Extracted file /home/runner/work/pbench/pbench/agent/bench-scripts/test-bin/fio-histo-log-pctiles.py in 1164ms```

Hey @siddardh-ra this must be the issue for Analyze (python) failure

[dbutenhof]

I'm assuming these errors are due to changes in the scanners. They both seem to be legitimate, but not very interesting to us... and definitely not related to your changes.

The "Javascript" code problem seems to be related to the Tool Meister Prometheus support:

Error: Extraction failed in /home/runner/work/pbench/pbench/agent/templates/prometheus.yml with error found character '%' that cannot start any token. (Do not use % for indentation)

I haven't investigated, but I'm assuming this means we're jinja2 processing to expand the list of tools we're scraping, and the code analyzer doesn't understand that it's intentionally not pure YML. Operationally I suspect that means this should be prometheus.yml.jinja2 or something.

That is to say, this has nothing to do with your change, and may be configuration changes in the code scanner. We should fix this with another PR; and we can either force your change in regardless of these annoying messages, figure out how to prevent the scanner from complaining about this, or wait for that "fix" to go in.

For the Python scanner, this seems to be

Warning: Extraction failed in /home/runner/work/pbench/pbench/agent/bench-scripts/test-bin/fio-histo-log-pctiles.py with error Syntax Error

This one appears to be because despite the .py file it's really a bash script. I presume was because something in the Agent tests refers to the file with the Python file type and the legacy tests are mocking some simplistic behavior. (It could as easily have been a Python that just did print statements.)

Again, we need to figure out how to either reconfigure the scanners to not care, or fix the root issue, perhaps, as I mentioned, by rewriting this mock as Python.

[webbnh]

This is looking pretty much done, now. There are just a couple of lingering questions:

• Auditing details.
• Naming of SQL constraints for Alembic.
• My POST-with-key question. (@dbutenhof?)
• And the annoyance below. 😏

...but we don't have to gate the merge on any of them.

[npalaska]

Looks really good

[npalaska]

The default required value is False but I guess it's fine to be explicit.

[dbutenhof]

Sorry, but I really don't like defining a POST schema parameter just to reject it, and there happens to be a better way... 😁

[dbutenhof]

Looks like it's only failing due to #3417.

[npalaska]

Looks like it's only failing due to #3417.

It's merged now, Siddardh you can rebase on main now.

[siddardh-ra]

Looks like it's only failing due to #3417.

It's merged now, Siddardh you can rebase on main now.

Done. Rebased 👍

[webbnh]

Looks great.