Why is the same code above the while cycle and inside it? You can remove bc = BodhiClient and try-except above the cycle, because you'll do it anyway inside the cycle.

This can cause the script to get stuck in an endless loop, if there's something wrong with the csrf retrieval. Why do you have it in a loop in the first place, are there any reasons that it should fail? And if it fails and it's an auth error, shouldn't you ask for the name/password again? And if it fails X times in a row (non-interactively), you should just exit, no reason to continue endlessly.

Okay, I was mind locked to a terrible code during debugging :) . Should be better now? I've left it in separate commit for review purposes, will squash before merge.

Also, I think it's okay to ask only for password on failed auth, but I don't have a strong opinion here. So, if you want, I can change the code so it asks user for both, username and password, on auth error.

The loop looks good now.

So, the bc.csrf() method asks for password? If it does, I guess it would be nice to ask for username if it fails, because the username is by default inherited from the system and could be different. You can pre-fill the username prompt with its current value, so if the user doesn't need to change username, he just presses Enter.

One more thing to consider, though. Just yesterday I encountered a problem that I walked away from FEK for some time, and when I returned back and wanted to submit some karma, it told me (roughly) "CSRF token expired", and the whole FEK crashed. It's the same as with Bodhi website, if you let it open for a long time, you need to refresh it before submitting karma. I would expect FEK to renew the token when needed and not crash. Will your patch fix this?

So, the bc.csrf() method asks for password? If it does, I guess it would be nice to ask for username if it fails, because the username is by default inherited from the system and could be different. You can pre-fill the username prompt with its current value, so if the user doesn't need to change username, he just presses Enter.

Yes, bc.csrf() asks for password if needed (if there is valid and usable openid cache file, it will return csrf and proceed). Okay, I'll change it to ask also for username on auth fail.

One more thing to consider, though. Just yesterday I encountered a problem that I walked away from FEK for some time, and when I returned back and wanted to submit some karma, it told me (roughly) "CSRF token expired", and the whole FEK crashed. It's the same as with Bodhi website, if you let it open for a long time, you need to refresh it before submitting karma. I would expect FEK to renew the token when needed and not crash. Will your patch fix this?

Yeah, makes sense, but debugging is going to be... "fun" :) . I'd rather handle this in a separate PR if you are okay with that?

... I walked away from FEK for some time, and when I returned back and wanted to submit some karma, it told me (roughly) "CSRF token expired", and the whole FEK crashed ... Will your patch fix this?

Looking at the code, my guess is "No", since the CSRF token/authentication is only performed in __init__(), the https://pagure.io/fork/frantisekz/fedora-easy-karma/blob/57b98f2c3ddd8d68901398d5ee8c5d43f4df713d/f/fedora-easy-karma.py#_790 method will either crash the same, or just do nothing anyway. There is no re-auth-code present as far as my understanding of the code goes.

Yeah, makes sense, but debugging is going to be... "fun" :) . I'd rather handle this in a separate PR if you are okay with that?

Sure, no problem.

... I walked away from FEK for some time, and when I returned back and wanted to submit some karma, it told me (roughly) "CSRF token expired", and the whole FEK crashed ... Will your patch fix this?

Looking at the code, my guess is "No", since the CSRF token/authentication is only performed in init(), the https://pagure.io/fork/frantisekz/fedora-easy-karma/blob/57b98f2c3ddd8d68901398d5ee8c5d43f4df713d/f/fedora-easy-karma.py#_790 method will either crash the same, or just do nothing anyway. There is no re-auth-code present as far as my understanding of the code goes.

Yeah, after looking at the code, I was thinking about raising exception to caller, reinitializing bc and returning back to try again at a point where it failed on comment submit. Doesn't seem nice though...

@frantisekz: This is what happens (roughly) when the csrf token expires - there is an unhandled exception in send_comment() that causes it to crash:

>>> from bodhi.client.bindings import BodhiClient
>>> bc = BodhiClient(username='jskladan', useragent='Fedora Easy Karma/GIT')
>>> csrf=bc.csrf()
Password: 
>>> bc.csrf_token
'7d0f559c9b169fc7c0c002110c9525e77ddfc4ee'
>>> _ = bc.comment('FEDORA-2019-70000e96a4', 'Test', karma=0)
>>> bc.csrf_token = '7d0f559c9b169fc7c0c002110c9525e77ddfc4ef'
>>> bc.comment('FEDORA-2019-70000e96a4', 'Test2', karma=0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.7/site-packages/bodhi/client/bindings.py", line 153, in wrapper
    raise BodhiClientException(problems)
bodhi.client.bindings.BodhiClientException: CSRF tokens do not match.  This happens if you have the page open for a long time. Please reload the page and try to submit your data again. Make sure to save your input somewhere before reloading. 

This might be an easy-fix, for the "expired csrf token situation":

>>> bc._session.cookies.clear()
>>> bc.csrf_token = None
>>> bc.csrf()

Thanks @jskladan , I've implemented it into the PR.

Please log/print the exception and say something like "Trying again"

And/or log the error here. I'm not exactly clear on what originates where, but in general you always want to have information about the true issue logged somewhere, with a detailed error message. If you rebrand everything as "Authentication error", you'll lose information about what exactly it was (CSRF, invalid password, etc).

In general the patch looks fine to me, just logging should be improved.

Logging should be good now. I'll leave code at the start to say Authentication Error, it should always mean that username/password is wrong.

I've added csrf token mismatch message if workaround code is hit. In this case, user shouldn't need to enter password again (openid cache should be there, just csrf needs refresh), but being more verbose doesn't hurt I guess :)

Maybe at least try to have a look at the error message. The BodhiClientException is quite "all-catching", and can possibly mean just about anything.

The BodhiClientException is quite "all-catching", and can possibly mean just about anything.

I wouldn't parse the message, just print it all into terminal and say "Trying again". We don't handle other cases anyway (at the moment). And the full exception message will help us with future bug reports.

I wouldn't parse the message, just print it all into terminal and say "Trying again". We don't handle other cases anyway (at the moment). And the full exception message will help us with future bug reports.

I don't care either way, I'm just pointing out that the warning message is (can be) misleading, and that running the "reset the session" lines might have nothing to do with what the error was in the first place.

The other thing that does not make sense (to me) is that the "BodhiClientException" is handled outside of the send_comment() method. I don't see the need of making this a special citizen. @frantisekz please move the BodhiClientException handling to send_comment().

IMO the proper(ish) solution would be in this ballpark:

    def send_comment(self, bc, update, comment, karma):
        orig_retries = bc.retries
        bc.retries = 1
        for retry in range(0, self.options.retries + 1):
            try:
                res = bc.comment(update["updateid"], comment, karma=karma)
                bc.retries = orig_retries
                return (True, res)
            except fedora.client.AuthError as e:
                return (False, 'Authentication error')
            except fedora.client.ServerError as e:
                self.warning("Server error: %s" % str(e))
            except BodhiClientException as e:
                self.warning("Bodhi Client error: %s" % str(e))
                if 'csrf' in str(e).lower():
                    self.warning("Probable CSRF token error. Trying to obtain a new one)
                    bc._session.cookes.clear()
                    bc.csrf_token = None
                    bc.csrf()

        bc.retries = orig_retries
        return (False, 'too many errors')

Note that the code above is "pseudocode" and most probably has typos, sorry...

Yep, makes sense @jskladan . Thanks!

The error is not logged anywhere. If there is no "csrf" in the message, you just silently ignore it (the worst thing to do). The original Josef's pseudocode logged the error first (the full message received), and then acted on it. Please add error logging.

LGTM now