Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review panel not working - Gerrit 3.8.2 #57

Open
svogt opened this issue Mar 25, 2024 · 26 comments
Open

Review panel not working - Gerrit 3.8.2 #57

svogt opened this issue Mar 25, 2024 · 26 comments

Comments

@svogt
Copy link

svogt commented Mar 25, 2024

Hi,

I get an authentication error when trying to open a change in the review panel. Log messages:

GET request to "https://<gerrit-host>/r/a/changes/<repo>~master~<change-id>/revisions/current/mergeable"
HTTPError: Response code 403 (Forbidden) 403 [object Object] Forbidden

Other calls work fine, also opening that URL in the browser is working fine (also using the same URL on CLI with curl works).

Not sure what the issue is, tried with the http password and with the auth-cookie - same result.

OS: WSL on Windows with Ubuntu 22.04

@SanderRonde
Copy link
Owner

Hmm that request seems to get rejected by gerrit for a number of reasons (a change already being merged being one of them). I'll make the extension continue on with execution (and the showing of the panel) even if that request fails.

@SanderRonde
Copy link
Owner

Should be fixed in the newest version (1.1.29)

@svogt
Copy link
Author

svogt commented Mar 26, 2024

updated, still same issue. review panel does not show the review I selected.

also I noticed another error in the Gerrit log:

GET request to "https://<host>/r/a/accounts/self"
HTTPError: Response code 403 (Forbidden) 403 [object Object] Forbidden
Invalid response

What I noticed in the browser that if I open that URL without an authentication cookie I get a denied immediately (no basic auth popup). Could it be that this endpoint need "preemptive auth"? Maybe the other one with the mergeable as well?

I can work around this if I add the "GerritAccount" cookie as the list of always to send cookie. But then I have to remove the username / HTTP password from the settings. Of course I'd rather prefer the user/pass option than to update the settings every time the Gerrit server is restarted or after 30 days when the cookie is renewed.

FYI: I'm also the Gerrit server admin - so I can check the server if you need logs from the backend. Can also do some debugging.

Unfortunately I cannot reopen this issue.

@SanderRonde
Copy link
Owner

What I noticed in the browser that if I open that URL without an authentication cookie I get a denied immediately (no basic auth popup). Could it be that this endpoint need "preemptive auth"? Maybe the other one with the mergeable as well?

Yes every API call under /a essentially requires "preemtive auth" in the sense that you need to have already been authenticated either through your cookies or through basic auth. It's not meant for direct access with the browser like the regular UI is. The gerrit documentation also mentions that 403 is returned when self is mentioned and the call was done without authentication. So that does explain why these calls specifically are failing.

I can work around this if I add the "GerritAccount" cookie as the list of always to send cookie. But then I have to remove the username / HTTP password from the settings. Of course I'd rather prefer the user/pass option than to update the settings every time the Gerrit server is restarted or after 30 days when the cookie is renewed.

I'm wondering what you mean with "add the "GerritAccount" cookie as the list of always to send cookie". That's what setting the cookie authentication setting does right? Or do you mean entering the username/password combo and on top adding GerritAccount to extraCookies?

FYI: I'm also the Gerrit server admin - so I can check the server if you need logs from the backend. Can also do some debugging.

That's nice, makes debugging a bit easier :) To find out what is causing this, let's rule out a couple of things:

  • It could be related to the extension or it could not be. The easiest way to test that is to make a request with Postman (or some other API client) that does the same thing the extension does. Namely a request to that same URL using either basic auth or cookies.
  • What I find curious is that cookies do seem to work and basic auth does not (am I correct in concluding that?). Can you confirm that that is the case? And is that also the case when using postman? Is there maybe some setting you disabled?

@SanderRonde
Copy link
Owner

Ah I just noticed you mentioned already testing this with CURL in the other issue... I'll come up with something new to test then

@SanderRonde
Copy link
Owner

So just curl --url {host}/a/accounts/self --header 'Authorization: Basic {toBase64(username:password)}' works for you but the extension does not? If so maybe something's going wrong in the request library

@svogt
Copy link
Author

svogt commented Mar 27, 2024

#58 is not exactly the same. The .../mergeable URL is working with curl. the .../accounts/self indeed is only working in the browser if I'm already logged in. I'll check if I can find a reason for that. Curl is working with .../accounts/self if I supply --cookie 'GerritAccount=<token>' on the command line.

I'm wondering what you mean with "add the "GerritAccount" cookie as the list of always to send cookie". That's what setting the cookie authentication setting does right? Or do you mean entering the username/password combo and on top adding GerritAccount to extraCookies?

I meant that using the cookie authentication setting AND / OR (tried both) username/password did not work for the review panel to show up. What DID work was

  • no username / password
  • cookie authentication setting is set
  • Add GerritAccount=<token> to the extra cookies.

IIRC it was not sufficient to only set the cookie authentication setting, only after adding it to the extra cookies it worked (with worked I mean I was able to get the review panel to show the comment box and the +1 / +2 settings.

That being said, even with those settings from above we are NOT able to post a comment, that one still fails.

@svogt
Copy link
Author

svogt commented Mar 27, 2024

For reference - asked about the accounts/self issue here: https://groups.google.com/g/repo-discuss/c/ne94hMyWiyw

@svogt
Copy link
Author

svogt commented Mar 27, 2024

That being said, even with those settings from above we are NOT able to post a comment, that one still fails.

Just noted that maybe the logging is just off as there are always an accounts/self call in close proximity to the error (so if the calls are done async could be that the error belongs to the other call, not the mergeable one)

The error for the post is

POST request to "https://<host>/r/a/changes/<repo>~master~I2393da4a9cb22232db7b444e1cc96915cf3338d8/revisions/20c439a327c3345880fb7f24af8cc44efc3b6ba5/review"
HTTPError: Response code 403 (Forbidden) 403 [object Object] Invalid authentication method. In order to authenticate, prefix the REST endpoint URL with /a/ (e.g. http://example.com/a/projects/).

Invalid response

@SanderRonde
Copy link
Owner

I've built a debugging version of the extension so I can better see what might be causing it. I've attached it as a zip-file since github does not allow uploading .vsix files. To use it, rename it to .vsix and install it in VSCode (use the ... in the extensions panel). Then after launching your editor, open the VSCode dev tools (Help -> Toggle developer tools) and the logging will all be prefixed with [GERRIT]. Can you please let me know what it logs in these scenarios?

  • With basic auth
  • With just the cookie setting
  • With the cookie setting and a manual cookie override

It'll basically just to a request to /accounts/self and log all the used headers/cookies.

(of course feel free to censor the actual contents of sensitive fields)

vscode--gerrit-1.2.28.zip

@DarinVenkovSEE
Copy link

Hi, I tried getting the debug output. Hope it worked. Wasn't sure if I should expand the Objects. If you need anything else please let us know.

  1. With basic auth:
    With basic auth
  2. With just the cookie setting:
    With just the cookie setting
  3. With the cookie setting and a manual cookie override:
    With the cookie setting and a manual cookie override

@SanderRonde
Copy link
Owner

Ahh I've found the issue regarding the cookie setting not actually being used. Something with VSCode secretly handing over a readonly proxy of an object. I've attached a zip file where that issue is fixed.

I'm still not actually sure why the regular HTTP API isn't working but let's first confirm that this works and then we can tackle that :)

vscode--gerrit-1.2.28.zip

@DarinVenkovSEE
Copy link

Hi, I installed the new version and tried the second use case again.

With just the cookie setting:
With just the cookie setting 2

But the result is the same. The cookie is missing.
Can you tell by the debug if I have the new version ?
Or did I not install it correctly ?

@svogt
Copy link
Author

svogt commented Apr 5, 2024

unfortunately neither with the attached Zip nor with the 1.2.29 version I'm able to post a comment.

GET request to "https://git.seeburger.de/r/a/accounts/self"
POST request to "https://git.seeburger.de/r/a/changes/<repo>~master~I2393da4a9cb22232db7b444e1cc96915cf3338d8/revisions/20c439a327c3345880fb7f24af8cc44efc3b6ba5/review"
HTTPError: Response code 403 (Forbidden) 403 [object Object] Invalid authentication method. In order to authenticate, prefix the REST endpoint URL with /a/ (e.g. http://example.com/a/projects/).

Invalid response
GET request to "https://<host>/r/a/changes/I4df7714d8f7c0d0629a3b8ae69566987e5a849ef/detail/"
POST request to "https://<host>/r/a/changes/<repo>master~I2393da4a9cb22232db7b444e1cc96915cf3338d8/submit"
HTTPError: Response code 403 (Forbidden) 403 [object Object] Invalid authentication method. In order to authenticate, prefix the REST endpoint URL with /a/ (e.g. http://example.com/a/projects/).

Invalid response

the two occurences of each are one for the click on "Submit patch" and one for "Send"

@SanderRonde
Copy link
Owner

Hmm I can't tell from the logs whether the zip file came across properly. I've re-packaged it to be sure. This time I've changed the [GERRIT] logs to [GERRIT]2 so we can tell.

vscode--gerrit-1.2.28.zip

@svogt
Copy link
Author

svogt commented Apr 11, 2024

I tried it, where should I see the 2? Was unable to find it, though it installed successfully and it also shows that it's outdated and could be updated to 1.2.29 but I do not see the 2 in the logs

@SanderRonde
Copy link
Owner

In the developer tools it spits out some logs starting with [GERRIT]. Those should now be [GERRIT]2.

Are you sure it installed succesfully? Maybe uninstall the previous version first and then re-install the version I attached?

@DarinVenkovSEE
Copy link

Hi, I just tried the latest zip and I can see the new logs starting with [GERRIT]2.
And this version worked and was sending the Gerrit authentication cookie in the request without me needing to configure it a second time as Extra Cookies:

image

@svogt
Copy link
Author

svogt commented Apr 11, 2024

So whats IMHO still open:

  1. using http password instead of the cookie
  2. submitting comments (i do not get log output for the post with the review comment)

image

@SanderRonde
Copy link
Owner

Hmm I'm leaning towards this being an issue with Gerrit itself. What do you think? As you describe in your google groups post it works in an authenticated browser (aka cookie), via CURL with a cookie (again cookie) but not when using basic auth via CURL. That sounds like it's broken for any HTTP-requesting program, whether it be CURL or my extension.

Submitting comments silently failing is sort of expected, it does require accounts/self for it to work properly (see line). It relies on the "/accounts/self call does not work" error to be shown before the user even gets to the point of submitting comments.

I could pour quite a bit of time into making it so there's another method of inferring accountID but I feel like this is something that is a bit of an edge case and should really be fixed by the gerrit team itself so I would avoid adding (for example) yet another setting to this extension just for an edge case that should be solved elsewhere. I do wonder what you think though

@svogt
Copy link
Author

svogt commented Apr 15, 2024

sorry if I wasn't clear ;) posting comments fails also with the auth-cookie even if account/self is working :) Yes, regarding the issue with the http pass. But if you only need the accountId we could also set that via settings. At least that one won't change (in contrast to the account cookie which will expire eventually)

@SanderRonde
Copy link
Owner

Ahh alright. I've got another debug version for you to try. This one has a bunch of logging around the posting-comments process. Can you let me know what it spits out? The logging is again in the devtools and contains [GERRIT].

vscode--gerrit-1.2.29.zip

@DarinVenkovSEE
Copy link

Hi, thank you for the new debug version.
I did a new test with the cookie setting and a manual cookie override.
(this is the same setting where the call to account/self is working but the POSTs are still not)
image

@SanderRonde
Copy link
Owner

Ahh it's the request itself failing. Here's another version then to log why that is happening.
vscode--gerrit-1.2.29.zip

@DarinVenkovSEE
Copy link

Hi, here is the debug:

image

@SanderRonde
Copy link
Owner

Ah yeah again an authentication issue. Not really much I can do against this... At least the accountID issue could have been put into a setting but an action like this can't. Or do you have any other ideas?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants