GeoServer
  1. GeoServer
  2. GEOS-4421

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. GEOS-4421.patch
        11 kB
        Andrea Aime
      2. GEOS-4421-a.patch
        13 kB
        Justin Deoliveira
      3. GEOS-4421-a.patch
        26 kB
        Justin Deoliveira
      1. filters.png
        72 kB

        Activity

        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        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
        Andrea Aime added a comment -

        Yep, looks good

        Show
        Andrea Aime added a comment - Yep, looks good
        Hide
        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
        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
        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
        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
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: