GeoServer

SpringDelegatingFilter interferes with security

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: None
  • Fix Version/s: 2.1.x
  • Component/s: None
  • Labels:
    None
  • Number of attachments :
    4

Description

The filter picks up and applies all security related filters declared in the app-context.
However those filters need to be applied in a specific order, and should not be used at all when we disable the security subsystem following the instructions at http://docs.geoserver.org/latest/en/user/security/sec_disable.html

Recommended solution: have the delegating filter look for a marker interface, not just any filter. Something like GeoServerPluginFilter or the like

  1. Text File
    GEOS-4421.patch
    14/Mar/11 10:25 AM
    11 kB
    Andrea Aime
  2. Text File
    GEOS-4421-a.patch
    20/Apr/11 9:23 AM
    13 kB
    Justin Deoliveira
  3. Text File
    GEOS-4421-a.patch
    15/Mar/11 9:22 AM
    26 kB
    Justin Deoliveira
  1. filters.png
    72 kB
    14/Mar/11 5:46 AM

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Andrea Aime added a comment -

Btw, even when things don't break the delegating filter makes every filter in the spring security chain be applied twice

Show
Andrea Aime added a comment - Btw, even when things don't break the delegating filter makes every filter in the spring security chain be applied twice
Hide
Permalink
Andrea Aime added a comment - - edited

A screenshot showing which filters get into the delegating filter. Afaik none of them is really supposed to be there, they all seem related to security.
So using a marker interface should not affect any other functionality in GeoServer (maybe monitoring is rolling out its own filters?)

Show
Andrea Aime added a comment - - edited A screenshot showing which filters get into the delegating filter. Afaik none of them is really supposed to be there, they all seem related to security. So using a marker interface should not affect any other functionality in GeoServer (maybe monitoring is rolling out its own filters?)
Hide
Permalink
Andrea Aime added a comment -

Hi Justin,
here is a tentative patch for this.
I moved the filter in main along with the marker interface to avoid dependency loops (as web-app might have to depend on modules using GeoServerFilter), modified the filters in monitoring and openplans-auth (they were the only ones I could find that are not declared in web.xml) and added a subclass of the open session in view filter that implements the marker interface.

Good to go?

Show
Andrea Aime added a comment - Hi Justin, here is a tentative patch for this. I moved the filter in main along with the marker interface to avoid dependency loops (as web-app might have to depend on modules using GeoServerFilter), modified the filters in monitoring and openplans-auth (they were the only ones I could find that are not declared in web.xml) and added a subclass of the open session in view filter that implements the marker interface. Good to go?
Hide
Permalink
Justin Deoliveira added a comment -

Patch looks great Andrea. Here is a little update that updates dbconfig module as well.

Show
Justin Deoliveira added a comment - Patch looks great Andrea. Here is a little update that updates dbconfig module as well.
Hide
Permalink
Andrea Aime added a comment -

Justin, I see this one is still open. What do you say, commit on 2.1.x or wait for the 2.1.0 release?

Show
Andrea Aime added a comment - Justin, I see this one is still open. What do you say, commit on 2.1.x or wait for the 2.1.0 release?
Hide
Permalink
Justin Deoliveira added a comment -

Hmmm... not sure. This close to 2.1.0 I am tempted to wait. But if you feel strongly then go ahead and commit it.

Show
Justin Deoliveira added a comment - Hmmm... not sure. This close to 2.1.0 I am tempted to wait. But if you feel strongly then go ahead and commit it.
Hide
Permalink
Andrea Aime added a comment -

Justin, I wanted to apply this patch but the GEOS-4421-a.patch makes no sense? Is it a revert patch? It also contains a lot of .orig files, as if there had been a conflict?

Show
Andrea Aime added a comment - Justin, I wanted to apply this patch but the GEOS-4421 -a.patch makes no sense? Is it a revert patch? It also contains a lot of .orig files, as if there had been a conflict?
Hide
Permalink
Justin Deoliveira added a comment -

Yeah... it does not make any sense... sorry about that. I will try to update the patch asap.

Show
Justin Deoliveira added a comment - Yeah... it does not make any sense... sorry about that. I will try to update the patch asap.
Hide
Permalink
Justin Deoliveira added a comment -

Ok, this patch should make more sense.

Show
Justin Deoliveira added a comment - Ok, this patch should make more sense.
Hide
Permalink
Andrea Aime added a comment -

Yep, looks good

Show
Andrea Aime added a comment - Yep, looks good
Hide
Permalink
Andrea Aime added a comment -

Soo... what about committing it? Shall I go ahead? 2.1.x too?

Show
Andrea Aime added a comment - Soo... what about committing it? Shall I go ahead? 2.1.x too?
Hide
Permalink
Justin Deoliveira added a comment -

Sounds good. +1 to committing to 2.1.x as well.

Show
Justin Deoliveira added a comment - Sounds good. +1 to committing to 2.1.x as well.
Hide
Permalink
Andrea Aime added a comment -

Patch applied on 2.1.x and trunk. The dbconfig module build fails for me btw, but I see that on Hudson the tests for community modules are not run anyways, and I checked by reverting the patch that I was getting the same 4 errors.
One of them is the same reported in GEOS-4562

Show
Andrea Aime added a comment - Patch applied on 2.1.x and trunk. The dbconfig module build fails for me btw, but I see that on Hudson the tests for community modules are not run anyways, and I checked by reverting the patch that I was getting the same 4 errors. One of them is the same reported in GEOS-4562
Hide
Permalink
Andrea Aime added a comment -

Mass transition all resolved issue that did not see any further comment in the last month to closed status

Show
Andrea Aime added a comment - Mass transition all resolved issue that did not see any further comment in the last month to closed status

People

  • Assignee:
    Justin Deoliveira
    Reporter:
    Andrea Aime
Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: