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

comment_controller: add skip_auth to rescue error block #5728

Merged
merged 1 commit into from
Jan 27, 2020

Conversation

sunny-b
Copy link
Contributor

@sunny-b sunny-b commented Jan 25, 2020

What type of PR is this? (check all applicable)

  • Bug Fix

Description

A Pundit::AuthorizationNotPerformedError in the CommentsController #create path (https://app.honeybadger.io/fault/66984/9508988843fe9435805fbb822bbe8242). It appears this is happening in the rescue error block since that is the only data path that doesn't use skip_authorize or authorize.

It's not immediately clear from HoneyBadger how this occurring and I wasn't able to reproduce the error naturally (I was able to repro it with binding.pry). But it appears an error is occurring before the authorization putting skip_authorization in the rescue block will ensure all data paths are covered.

Related Tickets & Documents

Closes #5717

Added to documentation?

  • no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

alt_text

cc: @mstruve @citizen428 @rhymes

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 25, 2020
@rhymes rhymes removed their request for review January 26, 2020 20:30
@rhymes
Copy link
Contributor

rhymes commented Jan 26, 2020

I'm honestly not entirely sure if this is the solution but I haven't fully understood the issue coming from HB I guess

Copy link
Contributor

@mstruve mstruve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me, I suspected that the error being raised had something to do with the rate limiter being triggered.

@rhymes basically the issue is that a common path, like the rate limit being hit, is raising a Honeybadger when it should just be returning an error to the user since it requires no action from us.

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 27, 2020
Copy link
Contributor

@citizen428 citizen428 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes, any exception we handle would trigger the after action and raise an authorization exception.

@@ -107,6 +107,8 @@ def create
rescue Pundit::NotAuthorizedError
raise
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR, but why are we rescuing from this exception only to immediately re-raise it?

If you call raise with no arguments, while inside of a rescue block, Ruby will re-raise the original rescued exception.

😕

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the exact same question when this was created, this is why.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mstruve, I'll literally make a PR to leave this as a comment in the code before we confuse more people with this in the future.

Copy link
Contributor

@Zhao-Andy Zhao-Andy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, one heck of an edge case. Thanks for the PR!

@mstruve mstruve merged commit 385a831 into forem:master Jan 27, 2020
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes PR: merged bot applied label for PR's that are merged labels Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: reviewed-approved bot applied label for PR's where reviewer approves changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle Pundit::AuthorizationNotPerformedError From Comments Create Action
5 participants