ewsterrenburg / python-otrs

Pythonic interface to OTRS SOAP API
GNU General Public License v3.0
47 stars 27 forks source link

Not able to re-use stored session_id properly #33

Closed StCyr closed 7 years ago

StCyr commented 7 years ago

Hi,

If you try to use an existing session_id (that you previously stored in a temporay file, for example), it will work OK unless the corresponding session_id doesn't exist (eg: has been removed) in the OTRS you're polling. In this case your SOAP operations will fail miserably.

python-otrs should be able to handle the case where a session_id fails to authenticate the SOAP operation and fall back to create a new session_id using regular login/password credentials.

Without such capability, python-otrs is not very fit for execution in cronjobs.

Here's an illustrative example of code I made to use python-otrs within a cronjob without creating hundreds of sessions in my OTRS instance every day:

# Open session with our ticketing system
server_uri = r'https://ticketing.fw.acme.org'
webservice_name = 'DefaultTicketConnector'
client = GenericInterfaceClient(server_uri, tc=GenericTicketConnectorSOAP(webservice_name))
# Try getting results using existing session_id. Fallback to create a new session when it doesn't work.
# TODO: This code doesn't handle the case when SessionCreate fails
try:
  session_file = open("/tmp/otrs_session","r")
  client.session_id = session_file.read()
  session_file.close()
  nb_new_tickets = len(client.tc.TicketSearch(States="new"))
  nb_unlock_tickets = len(client.tc.TicketSearch(States="open",Locks="unlock"))
except:
  session_id = client.tc.SessionCreate(user_login='read-only-soap-user', password='xxxxxxxxxxxx')
  session_file = open("/tmp/otrs_session","w")
  session_file.write(session_id)
  session_file.close()
  nb_new_tickets = len(client.tc.TicketSearch(States="new"))
  nb_unlock_tickets = len(client.tc.TicketSearch(States="open",Locks="unlock"))
mjducharme commented 7 years ago

I have two concerns about this type of approach:

  1. The OTRS developers intended the SOAP interface to behave exactly like the web interface does, giving the same errors at the same points, using the same permissions, etc. The web interface doesn't auto reauthenticate you when your session times out - it displays the login screen but you still must reauthenticate. This change would make the Python SOAP interface do something automatically that OTRS itself does not - this change of behavior should not be a default option, but could perhaps be a keyword argument to the GenericInterfaceClient initialization method.
  2. In your case it is just a cronjob but in many other cases there is an external system that is triggering these operations by launching the script directly, and it is very possible for two script instances to be launched at once. I can foresee undesirable race conditions taking place as a result, where a second script instance tries to use the dead session before the first script instance has realized that the session is dead. The problem with the approach of automatically reauthenticating and generating a new session ID silently is that you then need to check to see if the new session ID is the same as the one that was in the file originally and update it if it is not.

The only way I can see of avoiding such race conditions is to program additional logic into the script that is running python-otrs to begin with. The 'silent reauthenticate' that you are suggesting becomes potentially undesirable if you are developing special logic to handle this in certain cases.

As a suggestion, instead of running your ticket search for new and open tickets under the 'try' block, why not just run a simple test-case search that uses very low CPU in OTRS (ex. searching for ticket with ticket number given, where you know that ticket will always exist). If that single operation fails due to to session expiration then you know you have to reauthenticate. That is the way you would need to do it anyway to help avoid race conditions because if a larger script ran, silently reauthenticated the session on first operation, and only wrote the updated session at the very end of the script, then another script could easily run in the interim which would use the old session and create a second new session needlessly.

ewsterrenburg commented 7 years ago

Fully agree with @mjducharme. My reaction wouldn't have been this elaborate (thanks again ;), yet the summary would have been along the lines of "Please keep this logic in your own script".

You need some additional logic anyway (for instance to read and write the temp files). As far as I can see, passing an optional session_id to the client and would barely reduce your script, while reducing predictability of the library: should a session be created when a session_id is specified, yet there is no valid session (which is not what what normally happens when creating a client object...)? And if so, how should the session_id be returned to the initials script that initiates the client object?

Furthermore, it would not be sufficient in case of a long (permanently?) running script which would require a wrapper to precede each request with a check if the session is still valid (or keep track of "time since last request", also specify a value with the duration before a session times out and hope there's no other actions elsewhere interfering with your sessions, etc.).

So in short, it would add extra complexity which will only work for part of the scripts users could be coming up while the additional logic in their scripts would stay almost the same...

You could, as @mjducharme suggests, use a light weight check and modify your except to only catch the expected error in case of no session_id.

StCyr commented 7 years ago

Hi,

Thanks for your ( extensive :-) ) feedback. It all make sense.

Cyrille