tf / redmine_merge_request_links

Display links to associated Gitlab merge requests and GitHub pull requests on Redmine's issue page.
https://www.redmine.org/plugins/redmine_merge_request_links
MIT License
35 stars 26 forks source link

Hook executed successfully but returned HTTP 403 #21

Closed lakostin closed 4 years ago

lakostin commented 4 years ago

Started POST "/merge_requests/event" for 127.0.0.1 at 2019-12-13 18:50:01 +0300 Processing by MergeRequestsController#event as HTML ..... Current user: anonymous Completed 403 Forbidden in 1ms (ActiveRecord: 0.3ms)

tf commented 4 years ago

REDMINE_MERGE_REQUEST_LINKS_GITLAB_WEBHOOK_TOKEN/REDMINE_MERGE_REQUEST_LINKS_GITHUB_WEBHOOK_TOKEN set correctly?

lakostin commented 4 years ago

REDMINE_MERGE_REQUEST_LINKS_GITLAB_WEBHOOK_TOKEN/REDMINE_MERGE_REQUEST_LINKS_GITHUB_WEBHOOK_TOKEN set correctly?

Yep

tf commented 4 years ago

I guess the 403 comes from this line. You will have to debug why the verification fails in your setup.

lakostin commented 4 years ago

How to debug it?

lakostin commented 4 years ago

Is it ok, that the user is anonymous?

tf commented 4 years ago

Yes. The token verification replaces user authentication.

We should probably add some logging for these cases. The problem is that currently the token verification is implemented in each service specific event handler separately (e.g. GitHub even handler). So code that distinguishes between cases "token not configured"/"signature does not match" and logs warnings would need to be duplicated. I think it would be a good idea to extract some of this logic into a shared method that then passes the token to the event handler explicitly.

For your case, though, you could temporarily add some Rails.logger.info calls to the event handler of the service you are interested in your local copy of the plugin.

lakostin commented 4 years ago

I added Rails.logger.info(request) Rails.logger.info(event_handler.verify(request))

and here is the result.

Current user: anonymous

false Completed 403 Forbidden in 1ms

Not clear what to do with it...

tf commented 4 years ago

Add something like

Rails.logger.info("Token: #{@token}")
Rails.logger.info(request.headers['X-Hub-Signature'])

to the top of Redmine_mergeRequestLinks::EventHandlers::Github#verify

or

Rails.logger.info("Token: #{@token}")
Rails.logger.info(request.headers['X-Gitlab-Token'])

to the top of Redmine_mergeRequestLinks::EventHandlers::Gitlab#verify

lakostin commented 4 years ago

def verify(request) Rails.logger.info("xoxo") Rails.logger.info("Token: #{@token}") Rails.logger.info("xoxo") Rails.logger.info(request.headers['X-Gitlab-Token']) request.headers['X-Gitlab-Token'] == @token end

And here is the result... xoxo Token: xoxo hDHjcQPpW44K5bHMidYO xoxo false xoxo Token: xoxo hDHjcQPpW44K5bHMidYO xoxo

tf commented 4 years ago

So as I suspected initially, the REDMINE_MERGE_REQUEST_LINKS_*_WEBHOOK_TOKEN variables are not set.

The @token you logged is read from the environment here. So the fact that it is empty, means that the environment variable is not set for your webserver process.

lakostin commented 4 years ago

env variables are set but app still can't see it... ruby -e 'p ENV["REDMINE_MERGE_REQUEST_LINKS_GITLAB_WEBHOOK_TOKEN"]' "hDHjcQPpW44K5bHMidYO"

Anyway i can set it manually.

But here is another problem. Migration is not completed. Should i also do something to fix it? ActiveRecord::StatementInvalid (Mysql2::Error: Table 'easy.merge_requests' doesn't exist: SHOW FULL FIELDS FROM merge_requests):

tf commented 4 years ago

Did you run rake redmine:plugins:migrate RAILS_ENV=production like is says in the readme?

lakostin commented 4 years ago

After complete reinstall hook started to work finally :) but now the link is not appeared in the issue. is this plugin compatible with easy redmine?

tf commented 4 years ago

It sounds like you did not apply the Git patch for the Redmine source code which adds the hook to render the box.

Have you read the readme? I'm happy to help, but I find it a bit exhausting to walk you through each step here when everything I say is written in the readme.

Regarding Easy Redmine: I never used it. If the view patch applies then I guess it should work.

lakostin commented 4 years ago

I know, sorry. I read the readme carefully multiple times but it doesnt work just out of the box... To make it work each step should be debugged...

lakostin commented 4 years ago

patch was applied successfully btw

tf commented 4 years ago

No problem. I'm also happy to accept PRs that improve the readme or cover more edge cases. So it works now?

lakostin commented 4 years ago

no, hook works, mr table contains rows, patch is applied, but the link is not shown. i set "View associated merge requests" for all roles. Merge request links is enabled for the project. maybe you can send a screenshot so at least i know where to search for this link?

tf commented 4 years ago

It should display a box next to the journal items on the issue show page

image

lakostin commented 4 years ago

maybe you can help with debugging this case as well :)

lakostin commented 4 years ago

it now works finally!! easy redmine moved all pages to another place and kept all old pages. So the patch applied, but these changes actually were not displayed. Thank you very much for help! I think i should keep my own fork for easy redmine so the plugin could work correctly.

tf commented 4 years ago

Happy to hear that! :tada:

It'd be great if you could open a PR that adds a hint to the readme where the change of the patch needs to applied in the Easy Redmine case.

lakostin commented 4 years ago

ok. and environment variable was not applied because systemd hadn't seen it so it should be set in .service file.

tf commented 4 years ago

and environment variable was not applied because systemd hadn't seen it so it should be set in .service file.

This sounds like something specific to your deployment setup, though.