creating comment via API: cryptic error message about missing author #3298
Comments
|
Hi @decathorpe! I see some tracebacks on the server-side for this that might be helpful: and |
|
It may be helpful to inspect the POST that the Python client is sending as an example. |
|
Thanks! I tried running bodhi with the |
|
I've managed to "hack" the bodhi python client bindings to print the session and arguments when it does its POST requests, but now I'm stuck again. Because the python bindings send exactly what I expect them to. There is no mention of "author" or "user" anywhere, not even in the cookies that are set in the requests session. PS: The |
|
Additionally, the documentation also doesn't mention "author" or "user" at all: https://bodhi.fedoraproject.org/docs/server_api/rest/comments.html#service-1-POST |
|
@decathorpe I would expect the author to be known from the Pyramid session, though I honestly don't know a lot about how that works. Are you sending the same session info that the Python one is sending? It is not hard to make a local development environment with Vagrant: https://bodhi.fedoraproject.org/docs/developer/vagrant.html I recommend doing that so you can control the server side and inspect how it works. |
|
Looking at the bodhi code, pyramid is used exclusively on the server-side. The bodhi python client handles this simply via the requests session set up via fedora's OpenIDBaseClient. The session setup I'm doing works the same as the python OpenIDBaseClient, and should result in the same cookies being set. However, I found where the "missing author" error comes from: https://github.com/fedora-infra/bodhi/blob/develop/bodhi/server/models.py#L2872 It gets the "author" argument from here: https://github.com/fedora-infra/bodhi/blob/develop/bodhi/server/services/comments.py#L210 Which queries the pyramid request for either |
|
Good luck, and let me know what you find! I honestly haven't looked into how this works before, since it was already working when I started working on Bodhi and I've never had to mess with it. |
|
@decathorpe I just realized I should let you know that we are planning to switch Bodhi to OpenID Connect: #1180 I believe that @puiterwijk wants to get that done soon. |
|
@bowlofeggs okay, thanks for the heads-up. I guess I'll have to implement an OpenID Connect library in rust next 😂 |
|
Side note: This is not specific to my rust bindings. I just adapted fedora-easy-karma to the new bodhi API and now I'm getting the same "You must provide a comment author" error from fedora-easy-karma, which uses the first-party bodhi python bindings:
|
|
Here's a full traceback from fedora-easy-karma: It can be reproduced easily: @bowlofeggs Can you please look into this soon? Fixing fedora-easy-karma is quite high priority for QA, thank you. |
decathorpe
changed the title
|
Even the official CLI tool doesn't work: |
|
As a heads up, @bowlofeggs is away for 2 months and @abompard for 3 weeks, so we'll need to wait until somebody has time to look into this. |
Hmm, this just worked for me: Here's the comment it created: https://bodhi.fedoraproject.org/updates/FEDORA-2019-38fb6b744d#comment-968141 NB: this is bodhi-client-4.0.2-2.fc30.noarch, @kparal what's the version you use? |
|
Interesting, after I removed |
|
If that's the case, then there these are probably two different issues - since the rust code I'm using for OpenID authentication isn't using the session cache file at all. |
The old session file has 8 items in the list, the new one has 9 items. It seems the structure of the file changed, but no conversion is performed, and the existing code fails to work with the old one. |
|
@puiterwijk since this is openID auth stuff, were you involved by any chance? |
bowlofeggs
added
Client
|
So, is there any plan to move forward? This broke fedora-easy-karma which is quite a complication for updates testing. @abompard , are you the new bodhi maintainer? Can you look into this? |
|
A further note from @kparal on the 'downstream' issue: "My experience was that I had to clear cache every few days or weeks, the error seemed to pop up at random intervals." so presumably this isn't the 8 items vs. 9 items thing as you initially thought, but rather some kind of cache staleness issue? |
|
https://github.com/fedora-infra/bodhi/blob/develop/bodhi/server/services/comments.py#L216 doesn’t require the caller of this endpoint to be authenticated (no ‘factory’ argument), but fills ‘author’ based on the current authenticated session. So if you request any other endpoints that require auth, I predict it’ll start auth, and then commenting will work again until that session expires. |
|
heh, that's just about what I was figuring out right now, yeah. So how do you reckon we should fix this? Fix it on server end by requiring auth in the comment function? |
|
Aha - I think I might see when this broke. The whole 'allow anon comments with captcha' thing that the client goes to some trouble to deal with was removed from the server early this year, and that commit simplifies the bit @puiterwijk pointed to quite a lot, including taking out a check that 'author' is actually set. I think the fix here might simply be to restore that |
|
Well, if we no longer allow anonymous comments, the endpoint requires authentication to function.... so yeah, that’s the correct fix then. |
|
right, makes sense. I'll just send a PR to make the endpoint require auth. edit: hum, actually using an ACL factory isn't trivial because we don't have one for 'any authed user' (only for admins and packagers), and I'm not sure just restoring the edit again: Hum, given how openidbaseclient |
|
OK, finally managed to get a bodhi dev env running again (this is always a nightmare for some reason) and I'm pretty sure my |
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: fedora-infra#3298 Signed-off-by: Adam Williamson <awilliam@redhat.com>
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: fedora-infra#3298 Signed-off-by: Adam Williamson <awilliam@redhat.com>
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: fedora-infra#3298 Signed-off-by: Adam Williamson <awilliam@redhat.com>
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>
|
So my change won't necessarily fix @decathorpe 's initial problem exactly, since it's intended to trigger the code the Python client library has to try and reauthenticate if it receives a 403 error from the server, but @decathorpe was not using the Python client library at least initially. Still, I think it is pretty likely @decathorpe had basically the same problem: trying to post a comment while anonymous. Armed with that knowledge I think he ought to be able to handle it in his client code and we don't necessarily need to do anything further server side. I suppose the text of the error message could be tweaked to say more specifically that the cause of the problem might be that the session isn't authenticated... wdyt, @decathorpe ? |
|
@AdamWill I think I was hitting the same issue for a different reason - because I should always be already authenticated when hitting that API, due to a different library design in my fedora/bodhi bindings. But it turned out that the OpenID authentication isn't working correctly yet client-side (probably my fault) - and I still need to investigate why (the corresponding code in python-fedora reads like hedge maze that hasn't been tended for 10 years, so it's gonna take some time). But thanks for keeping me in the loop :) EDIT: I think I've fixed the first issue now (by manually handling the cookies correctly). The "You must provide a comment author" is now only displayed if the user has not successfully authenticated (e.g. if providing a wrong username or password). Now I'm getting CSRF token mismatches when trying to do stuff like create comments ... let's see if I can fix that next 😿 |
|
For the record: In the meantime, I managed to get things to work correctly. My rust bindings for the REST API are now working and feature-complete 🎉 (and I already built a working fedora-easy-karma replacement and an alternative bodhi CLI client on top of them) Thanks again for everybody's help with debugging and fixing the original issue. |
|
Cool! Can we see them? :) |
Sure, all projects I mentioned are inside this GitHub org :) I might look into packaging them for fedora, as well, though I haven't done any rust packaging for fedora yet. |
|
Do you mind if I point it up for the Fedora QA mailing list and ask folks to give it a spin? |
|
@AdamWill Sure! Also, RFEs and issue reports are welcome. I've been using my tools for a few weeks now and most things should "just work" © |
|
@abompard it seems like this wasn't included in 5.1.1. Can we please include it in a version that will get deployed? fedora-easy-karma is broken until this fix is deployed :( If it doesn't get out soon I'll have to write a downstream patch for the client or something... |
I'm working on my rust bindings for the bodhi REST API, but I can't get the creation of comments via POST requests to work. I get the following error message from a session that successfully authenticated via OpenID:
However, if I add a
authorfield to the JSON body of the comment, I get a KeyError forauthor-userandusernamedon't work either.I see some other recent issues mentioning a missing "author", but these are from the bodhi python bindings, and I'm not sure it's related.
The text was updated successfully, but these errors were encountered: