We should probably avoid the timeout calculation in the case where the session is already expired.

It should be possible to disable the timeout, e.g. by setting it to zero.

Including a comma after the last config item will make future diffs prettier.

New config option needs to be documented.

Since the term for what we're implementing is "renewal timeout", it might be better to keep that term intact in our options and variables, e.g. SessionRenewalTimeout

So maybe something like this?

        if not session_data['expired']:
            renewal_cutoff = (session_data['start_ts'] +
                                context.opts['SessionRenewalTimeout'] * 60)
            if time.time() > renewal_cutoff:
                session_data['expired'] = True
                update = UpdateProcessor('sessions',
                                         data={'expired': True},
                                         clauses=['id = %(id)s OR master = %(id)s'],
                                         values={'id': self.id})
                update.execute()
                context.cnx.commit()

Note that while we have a lot of old code that uses paramstyle codes other than "s", and db.py has code to compensate, it is best to use "%(param)s" regardless of the value type.

@mikem all fixed and I add check when options is set up to 0 for disable it.
@tkopecek are you ok with current option name?

The renewal docs can go under General authentication options. I don't think it it needs its own heading. Also, the grammar could be improved.

Perhaps, "The number of minutes before sessions are required to re-authenticate. Set to 0 for no timeout."

@mikem I moved renewal docs and updated it.

@tkopecek are you ok with current option name?

yep, I agree

works for me

@mikem there is a problem when we are checking start_ts. It returns fault with expiration, when kojid is trying the relogin. I spoke about it with Tomas and his idea is that we can add 'last_expired_ts' column to session table and check this timestamp instead of start_ts. What do you think? Do we want to add new column to session table? Or do you have any other idea?

few notes:
- start_ts + timeout is constant time, so after first expiration it will always expire
- modification of start_ts via relogin is not suitable as we would lose information for absolute expiration.
- adding new column is extending busy sessions table, but impact shouldn't be that big

Yeah, pretty much the only way is to add a new field. I recommend naming it renew_time.

I'm wondering if it makes sense to start it out NULL. This makes sense to me since at the start it has yet to be renewed. OTOH, it does make our check slightly more complex (I think
cutoff = (renew_ts or start_ts) + timeout).

I don't think it would be terrible to just update start_time since the renew call must provide full credentials, but it would be nice to keep the data anyway.

I suppose it might also be interesting to record a renewal_count, but probably not worth bothering at the moment.

Probably worth adding the new field to getSessionInfo

There's no need to specify a field as "DEFAULT NULL". All fields default to null if there is no other default specified.

Overall looks good!

@mikem I spoked about add this new field to getSessionInfo, but our final reason for this was not add it to the getSessionInfo, because this is not anything what we want to show there. But if you have any idea, why we want to add it there, ok, I can add it.

getSessionInfo can only be used by:

• calls from the session itself
• admins

I don't see why we wouldn't treat it differently from start_time, which we expose. That said, it's fine to leave it out of this PR

When I brought up getSessionInfo, I hadn't quite realized how inconsistent the api was. I am hesitant to try to solve that here. You've rightly filed this as a separate issue in #3793

Given that, I think the best thing to do for this PR is to leave the getSessionInfo code alone for now, and only add the renew_ts field to the query in auth.py, which is basically where we were before the last commit (apart from the schema fix).

:thumbsup:

Commit 747f1782 fixes this pull-request