Fix session handling for ICAT backend #156
Conversation
- This will mean a valid session ID can be used straight away, instead of having to use the login endpoint to create a new session ID and use the API using that. This will help when prod users use session IDs which have come from other auth services
- Unit test was failing due to null session data being entered to the database
- This will prevent the same client object being used across different endpoints, which could be a security concern. This means that for each request, the client object is built with the session ID assigned each time, so each request checks that the user has a valid, active session ID
def14c7 to
f4a06ce
Compare
|
Force push was to remove a commit (to impose Black on the repo) which I'd already done on a different branch. Cleans up this PR a bit. |
common/python_icat_backend.py
Outdated
| if kwargs["client"]: | ||
| client = kwargs["client"] | ||
| else: | ||
| client = create_client() |
There was a problem hiding this comment.
I wonder if using the ternary operator here makes this shorter & more readable - thoughts?
client = kwargs["client"] if kwargs["client"] else create_client()
There was a problem hiding this comment.
I've just pushed a change to make use of the ternary operator, and I've read online about some of its other uses (definitely a useful thing). :)
|
Just something I've noticed during testing the API - if you login to the API via the session endpoints, then you restart the server, the session ID you previously were using is no longer valid. I presume this is to do with Python-ICAT? Or the expire time on the sessions? |
I've just been testing this and can't recreate what you're describing, but I'm pretty sure I've experienced it on this branch before I submitted this PR. I remember looking at the That being said, this change should allow you to use the session ID between server restarts of a few minutes (like when you're testing changes as you develop) so I'd hope you can see that within your testing, as that was part of the motivation of reopening the issue. |
This PR will close #135. See the comment on that issue to understand why it was reopened (also discussed on Slack today).
Essentially, before this change, a user would have to send a request to
/loginto get a session ID, and then use that session ID in future requests. It wouldn't matter if the user already had a valid session ID from elsewhere, because the client object wasn't being logged in until a request to/loginwas sent.Really, this is quite a simple change; the session ID was being assigned to the
clientobject in the wrong place. But it'll make dev manual testing a lot nicer (I'm all for things that make my life easier 😃) and will mean users that use session IDs from places likescigateway-authcan do so without having to create another session ID using this API.I've done the same Apache jMeter testing that I did back in #149 and it works (50 concurrent users over a 2 second ramp up period). All unit tests pass - I fixed one which failed because some of the session test data was being set as null.