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

Add note on avoiding DoubleRenderError in Rails to Readme #53

Closed
wants to merge 1 commit into from

Conversation

thomasklemm
Copy link
Collaborator

When a Pundit::NotAuthorizedError is raised in an after_action (Rails 4)/ after_filter (Rails 3), the response has already been rendered.

To avoid a DoubleRenderError being raised, the response body can be cleared before redirecting or rendering a new view.

Tested only in the browser in Rails 4.0. Fixes #52.

Please add a comment if you know another solution so we can work it in.

@thomasklemm
Copy link
Collaborator Author

If this works in Rails 4 and 3, should we incorporate that fix in the verify_authorized filter?

@jnicklas
Copy link
Collaborator

I think the use case where this matters is a bit slim. You probably want to do authorization before any action takes place, thus it doesn't really make sense to authorize in an after filter, or after any rendering at all really. In the case of errors raised due to authorization not occuring, as in verify_authorized, we want those errors to bubble, because that should never happen in a production setting.

@thomasklemm
Copy link
Collaborator Author

True, that note would only be useful in development. Let's leave it at the discussion here as a reference if someone googles the reason for the DoubleRenderError.

@ghost
Copy link

ghost commented Aug 26, 2013

👍

@thomasklemm
Copy link
Collaborator Author

@BigNerdRanchDan Ok with that? The DoubleRenderError might look a little confusing when it happens at first, but I hope people will find the missing authorize call (or missing skip statement for the verify_authorized filter) quickly.

@ghost
Copy link

ghost commented Aug 26, 2013

I think that @jnicklas is correct. Me personally, I would rather have the gem enforce authorization even in production so that I can capture an authorization exception and handle it with the rescue_from block. What if I want to display 404, not 500, when an authorization is intercepted (the same way Github works)?

That's said, you shouldn't concern the Pundit gem itself with Rails anyways (its a pure Ruby library to me). Perhaps a better solution is to extract some of the Rails specific code to a gem called "pundit-rails". In that gem we can experimemt with different ways of integrating Pundit into Rails, without polluting the core gem. Does that sound more reasonable?

@thomasklemm
Copy link
Collaborator Author

@BigNerdRanchDan A pundit-rails gem doesn't sound unreasonable, but I don't think this issue is important enough to justify the extra effort. When there's more Rails-specific stuff we can come back and add in this error handling as a standard, too.

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

Successfully merging this pull request may close these issues.

DoubleRenderError using after_filters
2 participants