When using local auth, admin_session_timedout fails with FAS not defined. The proposed solution is to check if using FAS or local and apply the same logic as in auth_logout

Let's simplify this a little and keep the auth config in the variable.

So something like:

auth = APP.config.get('PAGUR_AUTH')
if auth in ['fas', 'openid']: 
    ...
else:
    ....

One remark, otherwise looks good, thanks for catching this! :)

One question, would you like to check why this was caught by the tests and maybe fix it?

Also, could you see to make the commit title a little shorter.

A good practice is to make the commit summary 80 chars, then an empty line, then the rest of the description of the
commit using 80 chars long lines.

Checked why there was no test failure and I think there is no test case for admin_session_timedout, so I added one. But I also noticed that pagure.init is initialized with "fas" auth and only later it is changed to "local", so this makes the test environment in pagure.init slightly different from the normal run.

Also updated the commit.

Checked why there was no test failure and I think there is no test case for admin_session_timedout, so I added one.

Many thanks for this

But I also noticed that pagure.init is initialized with "fas" auth and only later it is changed to "local", so this makes the test environment in pagure.init slightly different from the normal run.

I'm not sure to follow you here, could you point me to the code?

Regarding the PR it looks good, there are a few pep8 corrections to do but I can do them after merging unless you want to check them.
Could you rebase your PR on the top of master? I try to keep the history linear :)

This is an interesting way to do the tests, did you see how I was doing it elsewhere?

Would you rather change the test to be like this? It will be more aligned with the other tests, but tests admin_session_timedout indirectly.

def test_admin_session_timedout(self):
    lifetime = pagure.APP.config.get('ADMIN_SESSION_LIFETIME',
                                        datetime.timedelta(minutes=15))
    td1 = datetime.timedelta(minutes=1)
    #session already expired
    user = tests.FakeUser(username='foo')
    user.login_time = datetime.datetime.now() - lifetime - td1
    with tests.user_set(pagure.APP, user):
        output = self.app.post('/settings/')
        self.assertEqual(output.status_code, 302)
    #session did not expire
    user.login_time = datetime.datetime.now() - lifetime + td1
    with tests.user_set(pagure.APP, user):
        output = self.app.post('/settings/')
        self.assertEqual(output.status_code, 200)

I'm not sure to follow you here, could you point me to the code?

When I start the tests pagure/tests/init.py will import pagure, that will run this block of code (because auth is fas)

if APP.config.get('PAGURE_AUTH', None) in ['fas', 'openid']:
    from flask_fas_openid import FAS
    FAS = FAS(APP)

Only later will run the code that sets the auth local

class PagureFlaskLogintests(tests.Modeltests):
    def setUp(self):
        [...]
        pagure.APP.config['PAGURE_AUTH'] = 'local'

because of this sequence FAS will be defined when running tests.

Regarding the PR it looks good, there are a few pep8 corrections to do but I can do them after merging unless you want to check them.
Could you rebase your PR on the top of master? I try to keep the history linear :)

Sorry for that. I will check and rebase.

Would you rather change the test to be like this? It will be more aligned with the other tests, but tests admin_session_timedout indirectly.

We could do both.

Note that you could follow_redirects=True in the request and check the message flashed instead or in addition to checking the code returned.

Note that you could follow_redirects=True in the request and check the message flashed instead or in addition to checking the code returned.

Tried the follow_redirects but I am having the problem that this user_set contextmanager will set back the user when running for the redirected page.

@contextmanager 
def user_set(APP, user):
    def handler(sender, **kwargs):
        g.fas_user = user

What happens when calling self.app.get('/settings/'):

1. contextmanager sets the user before the call to user_settings
2. admin_session_timedout will logout (and remove the flask.g.fas_user)
3. user_settings will redirect to auth_login
4. contextmanager will set again the flask.g.fas_user
5. because it is authenticated auth_login will redirect to user_settings
6. admin_session_timedout will logout .... it becomes a loop

If you have some idea to make this work I can try. Otherwise I will skip the use of redirect in the tests.

Ah, I see the problem indeed. Would you mind adding this as a comment in the test? I know myself and in two weeks I'll be asking myself that same question otherwise :)

Updated and rebased.

Cool, perfect, thanks! :)