Generation of API key on pbench-server

[siddardh-ra]

Generate a unique API key internally when the user presents a Keycloak access_token and encode the user identity in the API key.

Reworked active_token table.

This API Key will be also be validated under verify_auth module .

POST /api/v1/generate_key call generates api_key and will be updated in the reworked api_key (active_token)

[dbutenhof]

I started to review this before reading all of the main tab, but I realize you're not rebased on top of Nikhil's merge and you've got a lot of conflicts and obsolete code that you need to clean up before this is ready for review!

[webbnh]

POST /api/v1/generate_key call generates api_key and will be updated in the reworked api_key (active_token)

This API doesn't seem very resource-centric. In particular, I expect that there will be a desire for an authorized user to be able to fetch the values of existing keys. And, there will certainly be a need to be able to revoke/remove an existing key. So, we'll want a suite of methods:

POST /api/v1/access_key    ## Creation
GET /api/v1/access_key
DELETE /api/v1/access_key/<id>   ## Not sure what the "ID" should be...the key value is ungainly....

[npalaska]

Yeah, all the openid-connect changes are on the main now, you should rebase this on top of the main and that should minimize the conflicting files.

[dbutenhof]

Great start, Siddardh, but sadly, if not unexpectedly, I have comments... 😆

[webbnh]

Great start, Siddardh, but sadly, if not unexpectedly, I have comments... 😆

Yeah, what Dave said. 😉

A few overriding themes emerge:

• I think the API "resource" name should be a noun rather than a verb.
• What do we really want in the API key token values? (They are not OIDC/Keycloak tokens, so they don't have to have the same contents, I think.)
• Do we really want them to auto-expire (I think we do not).
• Some attention needs to be paid to what we log and how (e.g., we have a special exception for dealing with internal server errors, which we should use, and we should avoid calling abort() in deeply-nested functions).
• We need to support users having more than one key.

And, I've got a whole bunch of medium, small, and nit-level items.

[npalaska]

Okay, I glanced over the changes but there are already a lot of comments so I'll pass until you push another commit.

[webbnh]

Sorry to be posting/reviewing twice, but GitHub got confused and posted my previous review before I was ready....

It looks like you've addressed most of the items from my reviews from last week. However, you missed a couple of items: there's some odd code in test_verify_auth_api_key() (1, 2) and test_verify_auth_api_key_invalid() (3). And there is the stuff that I found in your updates.

Also, apparently, this PR now has a conflict in lib/pbench/server/api/__init__.py, which you'll need to address before Mr. Jenkins will touch it.

And, I found one more thing which I request you fix, below.

[siddardh-ra]

However, you missed a couple of items: there's some odd code in test_verify_auth_api_key() (1, 2) and test_verify_auth_api_key_invalid() (3). And there is the stuff that I found in your updates.

Addressed all the review comments 👍

And, I found one more thing which I request you fix, below.

Done

[dbutenhof]

The most important part is dropping the redundant audit CREATE; see the explanation in the comment. I'm also really on the edge about requesting that you set the completion audit attributes to record the key just in case we need to find it... but it's a value that's awkward to display.

[webbnh]

This is starting to look great! I think you've resolved all of my actual objections (although, I second Dave's, but I'll let him be the one to block the merge, now 😀), but I did find some more things which you should consider addressing.

[dbutenhof]

This is starting to look great! I think you've resolved all of my actual objections (although, I second Dave's, but I'll let him be the one to block the merge, now grinning), but I did find some more things which you should consider addressing.

It seems that Siddardh acknowledged and even "liked" several comments, but didn't make changes. We need to remove the duplicate and broken audit record. I'd really really like to see the audit log verified in the unit tests, plus adding (and verifying) the audit attributes to identify the key would be great. I really don't like some aspects of the exception handling because right now the APIKeyError doesn't actually mean much but somewhat hides the real cause.

And Webb's right that, if this handling is turning a duplicate key into an internal server error, that's likely to become unpleasant ...

[siddardh-ra]

It seems that Siddardh acknowledged and even "liked" several comments, but didn't make changes.

I've updated the changes related to Audit and also asserted that in unit tests as well and will push those changes.

And Webb's right that, if this handling is turning a duplicate key into an internal server error, that's likely to become unpleasant.

Yeah, this is not the right way to handle this scenario. Adding some changes to handle the SQL Related errors for a debugging

[webbnh]

Most of these changes are good; however, your attempt to refactor the SQL error decoding was not successful. I think we need to try a somewhat different approach. (I would be happy to discuss this with you, if you like.)

[webbnh]

I would be happy to discuss this with you, if you like.

I went ahead and captured my thoughts.

[dbutenhof]

We're getting really close here!

[webbnh]

Yes, I'm with Dave: we're getting really close. But, there are a few items (although small, at this point) which need touching up.

In particular, the call to decode_integrity_error() needs a little more work, and I recommend being a little more rigorous and concrete with its interface.

[webbnh]

This seems a little "heavy" (or not DRY enough). I think it would be better to use a local variable to capture the response status, which you would set to OK here and to CREATED either inside the try block after line 67 or in an else clause on the try block, and then have a common exit path where you create the response, set the status_code, and return.

[dbutenhof]

This is also skipping the audit attributes setting ...

Although ... this makes me realize that there isn't really a clear way to record the "duplicate, alternate success" status, and I'm not sure what to do about that. Probably nothing, in the context of this PR. 😦

[webbnh]

This is also skipping the audit attributes setting ...

Another excellent reason for a common return point!

I'm not sure what to do about that. Probably nothing, in the context of this PR. 😦

Yes, not in this PR.... 😏

[dbutenhof]

Just some commentary on recent changes and conversations, but I'm happy enough at this point to just get it in.

[dbutenhof]

This is also skipping the audit attributes setting ...

Although ... this makes me realize that there isn't really a clear way to record the "duplicate, alternate success" status, and I'm not sure what to do about that. Probably nothing, in the context of this PR. 😦

[dbutenhof]

This should probably be if decode_exc is e ... that is, it's not just Object.__eq__(e) (whatever logic the superclass __eq__ chooses, which might not be much) but the exact same exception instance. Although in the context of this logic I'm not sure it makes a functional difference since the other paths aren't likely to return an equal or identical instance. 😆

[webbnh]

Dave is, of course, correct -- we should be using is, not ==, to compare the two objects. (I had that in an earlier version of my suggestion, but I slipped up in my latest.)

[webbnh]

And this then prompts another tweak (which I'm not recommending for this PR, but...): since we're expecting decode_integrity_error() to construct exception objects for us in two out of the three cases, we might as well pass in the c'tor for the third case as well, and then the code here can go back to just raise'ing whatever it returns. 🙂

[webbnh]

This is good enough, now. 😀

It would be better with the is/== thing and the decode_integrity_error() signature fixed, and it would be nicer if decode_integrity_error() took an additional argument to construct the no-match return value, but I think we can merge this as it is, now (we can add the third factory to decode_integrity_error() when we convert the other three DB classes to use it).

[webbnh]

This is good, but I was hoping for something even more specific. 😏

    exception: IntegrityError, on_null: Callable[[str], Exception], on_duplicate: Callable[[str], Exception]

[webbnh]

Dave is, of course, correct -- we should be using is, not ==, to compare the two objects. (I had that in an earlier version of my suggestion, but I slipped up in my latest.)

[webbnh]

And this then prompts another tweak (which I'm not recommending for this PR, but...): since we're expecting decode_integrity_error() to construct exception objects for us in two out of the three cases, we might as well pass in the c'tor for the third case as well, and then the code here can go back to just raise'ing whatever it returns. 🙂

[webbnh]

This is also skipping the audit attributes setting ...

Another excellent reason for a common return point!

I'm not sure what to do about that. Probably nothing, in the context of this PR. 😦

Yes, not in this PR.... 😏

[dbutenhof]

Let's get this in, but a suggestion for a small task for @siddardh-ra ... we should add a functional test case that uses your POST /api/v1/key to generate an API key and validates that you can use it to see/update tester owned private datasets.

I hate the way our functional test system is getting forced into a single file in order to get test dependency ordering (because the @pytest.mark.dependency only works within a file); but it can be a separate class, and dependent on upload.