When adding comment, respond 403 if user is not authed

[AdamWill]

Since 2e6326e, which removed the code allowing anonymous
comments with a captcha, we have effectively been assuming the
session is properly authenticated when adding a comment - we
look at request.user and request.user.name for the 'author'
and don't do any checks, we just assume they were set. But if
we have a cached session that has expired, or something like
that, they will not be set. Without an explicit check, the
error the client ultimately gets when trying to add the comment
fails is not one which the client code recognizes as meaning
'reauthentication needed', so the client just sort of bails out.
This was breaking apps like fedora-easy-karma which just
retrieves a bunch of update info then tries to leave comments,
once the cached session for the client expired.

To fix this, we'll restore the check (removed in 2e6326e) that
author actually gets set, and tweak it a bit to return 403 not
400 if it wasn't set. The client code does recognize a 403
response as meaning 'reauth needed', so this fixes the problem,
and it's a more correct response than 400 anyway.

Resolves: #3298

Signed-off-by: Adam Williamson awilliam@redhat.com

[AdamWill]

I tested this manually, by setting up a Bodhi dev server, authing a client session, then just hand-editing the cache file and changing the auth token in it so it didn't work any more. After that, I could reproduce the bug at will, and confirm that this change solves it, both for an interactive session just using a BodhiClient instance, and a real fedora-easy-karma run.

[AdamWill]

Failures are because there isn't test coverage. I don't really know the test structure well enough to add tests for this, I can't see how in test_comments.py to trigger the failure case here.

[AdamSaleh]

@AdamWill you actually can test it, but it is a bit trickier because by default, all of the unit-tests will use the default guest/testing user and you'd find we use htis approach in i.e.test_verrides.py testcase test_override_view_not_loggedin ... the testcase I am pasting here works, feel free to use it (and probably clean it up a bit?) :)

    def test_comment_not_loggedin(self):
        """
        Test a non logged in User can't comment and will receive unauthorized error
        """
        import webtest
        import copy
        from bodhi.server import main
        anonymous_settings = copy.copy(self.app_settings)
        anonymous_settings.update({
            'authtkt.secret': 'whatever',
            'authtkt.secure': True,
        })
        with mock.patch('bodhi.server.Session.remove'):
            app = webtest.TestApp(main({}, session=self.db, **anonymous_settings))

        csfr =  app.get('/csrf', headers={'Accept': 'application/json'}).json_body['csrf_token']

        update = Build.query.filter_by(nvr='bodhi-2.0-1.fc17').one().update.alias
        comment = {
            'update': update,
            'text': 'Test',
            'karma': 0,
            'csrf_token': csfr,
        }
        res = app.post_json('/comments/', comment, status=403)
        assert 'errors' in res.json_body

[AdamWill]

Thanks, guy with excellently-chosen name! Yeah, I realized later yesterday it would make sense to just look at tests of other things that require auth and see how those are done. Thanks for going the extra mile for it though. I'll take your test and clean it up and update the PR.

[AdamSaleh]

Thanks for figuring out that return 403 will force re-auth :-)