Crowd Plugin

CrowdAuthSecurityFilters generates an IllegalStateException on response.sendError(403)

Details

  • Type: Bug Bug
  • Status: Resolved Resolved
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: Grails-Crowd 0.4
  • Fix Version/s: Grails-Crowd 0.5
  • Component/s: None
  • Labels:
    None
  • Patch Submitted:
    Yes

Description

If authorisation has been denied for a user, response.sendError(403) generates an IllegalStateException due to the response already being sent (I would assume this is because of the redirect to the login page).

Although the 403 seems to be a reasonable thing to expect, it is never interpreted or seen due to the exception and therefore doesn't really serve much of a purpose. I've included a patch that simply removes the sendError, but really I'm not sure if this issue might be resolved better somewhere else.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Landon Cheek added a comment -

This patch results in a security hole in the plugin: Though the response.sendError results in incorrect behaviour when a user has not yet authenticated, this response is necessary for when the user is authenticated and must then be authorised. If the sendError is missing, an unauthorised user is able to access areas that they should not.

Moral of the story: Do not apply this patch. The issue must be resolved elsewhere.

Show
Landon Cheek added a comment - This patch results in a security hole in the plugin: Though the response.sendError results in incorrect behaviour when a user has not yet authenticated, this response is necessary for when the user is authenticated and must then be authorised. If the sendError is missing, an unauthorised user is able to access areas that they should not. Moral of the story: Do not apply this patch. The issue must be resolved elsewhere.
Hide
Permalink
Landon Cheek added a comment -

This patch results in correct behaviour, as well as fixing the issue.

Show
Landon Cheek added a comment - This patch results in correct behaviour, as well as fixing the issue.
Hide
Permalink
Landon Cheek added a comment -

This patch results in correct behaviour, as well as fixes the issue.

Show
Landon Cheek added a comment - This patch results in correct behaviour, as well as fixes the issue.
Hide
Permalink
Landon Cheek added a comment -

Current patch solves the problem.

Show
Landon Cheek added a comment - Current patch solves the problem.
Hide
Permalink
Graham Bakay added a comment -

Hey Landon,

So changing line 273 to:

if (session.principalGroups && !session.principalGroups.containsAll(requireAuthorization)) {

will fix this error? In the patch it references line 138, but I think that is a spurious whitespace diff.

Could you confirm? Just tying all the loose ends for 0.5.

Show
Graham Bakay added a comment - Hey Landon, So changing line 273 to: 
if (session.principalGroups && !session.principalGroups.containsAll(requireAuthorization)) {
will fix this error? In the patch it references line 138, but I think that is a spurious whitespace diff. Could you confirm? Just tying all the loose ends for 0.5.
Hide
Permalink
Landon Cheek added a comment -

It's been a little while, but I believe that was the only line required to change.

For the life of me I can't remember what difference that actually made, or how I arrived at that solution.

Show
Landon Cheek added a comment - It's been a little while, but I believe that was the only line required to change. For the life of me I can't remember what difference that actually made, or how I arrived at that solution.

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.1#660-r161644) | Report a problem
Powered by a free Atlassian JIRA open source license for Grails project. Try JIRA - bug tracking software for your team.