Skip to content

#10923, #10924: Disabling GeoFence Rules and Add filter by IP for GeoFence rules#10925

Merged
tdipisa merged 9 commits intogeosolutions-it:masterfrom
mahmoudadel54:issue_10923_10924
Apr 4, 2025
Merged

#10923, #10924: Disabling GeoFence Rules and Add filter by IP for GeoFence rules#10925
tdipisa merged 9 commits intogeosolutions-it:masterfrom
mahmoudadel54:issue_10923_10924

Conversation

@mahmoudadel54
Copy link
Copy Markdown
Contributor

@mahmoudadel54 mahmoudadel54 commented Mar 12, 2025

Description

This PR includes:

  • adding IPAddress filter in rules manage grid
  • adding Date filter in rules manage grid for validity period fields
  • add validity period fields for rule insertion/update

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

Issue

#10923 #10924

What is the current behavior?
#10923 #10924

What is the new behavior?
IPAddress filter is added to rules manager grid and date filter as well.
Now to add rule user can enter validity period fields 'validAfter', 'validBefore' in insert new rule or update existing rule

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes, and I documented them in migration notes
  • No

Other useful information

Testing:

Can be tested on nightly geoserver/geofence.

GeoFence:

https://build.geoserver.org/geofence/nightly/main/geofence-main-latest-war.zip

GeoServer

https://build.geoserver.org/geoserver/main/geoserver-main-latest-war.zip
https://build.geoserver.org/geoserver/main/ext-latest/geoserver-2.27-SNAPSHOT-geofence-server-h2-plugin.zip

geosolutions-it#10924: Disabling GeoFence Rules

Description:
- adding IPAddress filter in rules manage grid
- adding Date filter  in rules manage grid for validity period fields
- add validity period fields for rule insertion/update
- add unit tests
- add translations
@mahmoudadel54 mahmoudadel54 added the New Feature used for new functionalities label Mar 12, 2025
@mahmoudadel54 mahmoudadel54 requested a review from MV88 March 12, 2025 00:09
@mahmoudadel54 mahmoudadel54 self-assigned this Mar 12, 2025
This was linked to issues Mar 12, 2025
geosolutions-it#10924: Disabling GeoFence Rules

Description:
- fix FE failure unit test
@offtherailz offtherailz removed the request for review from MV88 March 12, 2025 16:57
@offtherailz
Copy link
Copy Markdown
Member

offtherailz commented Mar 13, 2025

@tdipisa by they way.
I noticed that the documentation for the pages
https://mapstore.geosolutionsgroup.com/mapstore/docs/api/plugins#pages.RulesManager

Disappeared on dev

https://dev-mapstore.geosolutionsgroup.com/mapstore/docs/api/plugins#pages.RulesManager

Trying to apply tthe same config, I have this result

image
While I see on dev it works well:
image

So we have to update the doc properly to explain how to setup rule-manager.

The new correct config, copied from dev instance, is this:

"rulesmanager": [
      "Redirect",
      {
        "name": "BrandNavbar",
        "cfg": {
          "containerPosition": "header"
        }
      },
      "Home",
      "ManagerMenu",
      "Login",
      "Language",
      "RulesDataGrid",
      "Notifications",
      {
        "name": "RulesEditor",
        "cfg": {
          "containerPosition": "columns",
          "disableDetails": true
        }
      }
    ],

If documented somewhere, it should be linked in rules-manager plugin.
I'd document also how to obtain the ID of the geoserver instance, that is now not clarified.

I had to create an instance by sending the following post request

curl --location 'http://localhost:8080/geofence/rest/instances' \
--header 'Content-Type: application/json' \
' \
--data '{
  "name": "GeoServer",
  "description": "",
  "baseURL": "http://localhost:8080/geoserver",
  "username": "admin",
  "password": "geoserver"
}'

It returns the ID of the instance in plain text (e.g. 7)

You can get the current configured instaces with this call:

curl --location 'http://localhost:8080/geofence/rest/instances' \
--header 'Accept: application/xml' \
'

The ID should be used in configrued geoServerInstance in localConfig.json

Copy link
Copy Markdown
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

Testing this PR widely is very hard by per-se, because we should need to test many use cases:

  • Standalone new version (nightly)
  • Embedded new version (nightly)
  • Standalone old version (I suppose this tool needs to work with current and old versions of geoserver/geofence too)
  • Embedded old version (I suppose this tool needs to work with current and old versions of geoserver/geofence too)

@mahmoudadel54 did you have a chance to test all these use cases?

Moreover configuring everything is not easy. I had the opporunity, in the time I had dedicated on it, to test only the standalone version.
I had a lot of crashes to report, so I stopped reviewing after reporting 3 different crashes, waiting for a more stable version.

Testing all the version should not be up to me, but we could provide a structure to switch and test easely the various version to testers/develoers. If we want this to be maintaiable, a good idea can be to create a docker dedicated, or nice instructions to setup the test env.

Here the points I report.

  • See the comment for configuration and documentation above.
  • Because geofence improvemnts are still on nightly, we have to ensure that it will work also for existing version of geoserver with compatibility, I didn't test it, but we have to proof this change will not be distruptive. Maybe add an opt-in ?
  • I noticed this bug, not sure it is related to the PR, but on dev is not reroducible.
MapStore.mp4
StandardRouter.jsx:88 TypeError: Cannot read properties of null (reading 'scrollTop')
    at DataGrid.eval [as scrollListener] (DataGrid.jsx:29:35)
    at RulesGrid.eval [as componentDidUpdate] (RulesGrid.jsx:46:19)
  • The ipAddressAny parameter do not have any effect. I'd expect it to filter by precise IP (range) vs if the rule is applied. Verify if it makes sense, or if it doesn't show it checked and disable. The same looks to be
MapStore.1.mp4
  • See the video above (for ipAddressAny, I had a crash while recording .
StandardRouter.jsx:88 TypeError: Cannot read properties of undefined (reading 'id')
    at eval (react-data-grid-addons.js:9672:39)
    at Array.filter (<anonymous>)
    at CustomDragLayer.isDraggedRowSelected (react-data-grid-addons.js:9671:30)
    at CustomDragLayer.getDraggedRows (react-data-grid-addons.js:9690:29)
    at CustomDragLayer.renderDraggedRows (react-data-grid-addons.js:9702:20)
    at CustomDragLayer.render (react-data-grid-addons.js:9745:31)
    at finishClassComponent (react-dom.development.js:18407:31)
    at updateClassComponent (react-dom.development.js:18360:24)
    at beginWork$1 (react-dom.development.js:20108:16)
    at HTMLUnknownElement.callCallback (react-dom.development.js:362:14)
  • Another crash while playing on date filter.
MapStore.2.mp4
StandardRouter.jsx:88 TypeError: Cannot destructure 'state.filters' as it is undefined.
    at rulesmanager (rulesmanager.js:161:15)
    at combination (combineReducers.js:126:29)
    at reduce (StateUtils.js:256:14)
    at standardRootReducerFunc (defaultOptions.js:79:8)
    at rootReducer (StandardStore.js:90:12)

@tdipisa
Copy link
Copy Markdown
Member

tdipisa commented Mar 13, 2025

@offtherailz

@tdipisa by they way.
I noticed that the documentation for the pages
https://mapstore.geosolutionsgroup.com/mapstore/docs/api/plugins#pages.RulesManager

Disappeared on dev

https://dev-mapstore.geosolutionsgroup.com/mapstore/docs/api/plugins#pages.RulesManager

thank you for informing about that. I think we have to check why and restore, then. Please take care of this as part of this PR @mahmoudadel54, if possible.

@tdipisa tdipisa added this to the 2025.01.00 milestone Mar 13, 2025
@mahmoudadel54
Copy link
Copy Markdown
Contributor Author

mahmoudadel54 commented Mar 13, 2025

@offtherailz

  • yeah, the 1st issue is a bug not related to the work in this issue [I fixed it]
  • for the ipAddressAny parameter do not have any effect. you should use this link https://tehnoblog.org/ip-tools/ip-address-in-cidr-range/ which I added into the PR description above to validate your filter
  • for the crashing in ipAddressAny filter please give me clear steps to reproduce as I have tried many time but can't reproduce it
  • for the date filter by writing date manually --> I will fix it
  • I noticed this bug, not sure it is related to the PR, but on dev is not reroducible.

MapStore.mp4

StandardRouter.jsx:88 TypeError: Cannot read properties of null (reading 'scrollTop')
    at DataGrid.eval [as scrollListener] (DataGrid.jsx:29:35)
    at RulesGrid.eval [as componentDidUpdate] (RulesGrid.jsx:46:19)
  • The ipAddressAny parameter do not have any effect. I'd expect it to filter by precise IP (range) vs if the rule is applied. Verify if it makes sense, or if it doesn't show it checked and disable. The same looks to be

MapStore.1.mp4

  • See the video above (for ipAddressAny, I had a crash while recording .
StandardRouter.jsx:88 TypeError: Cannot read properties of undefined (reading 'id')
    at eval (react-data-grid-addons.js:9672:39)
    at Array.filter (<anonymous>)
    at CustomDragLayer.isDraggedRowSelected (react-data-grid-addons.js:9671:30)
    at CustomDragLayer.getDraggedRows (react-data-grid-addons.js:9690:29)
    at CustomDragLayer.renderDraggedRows (react-data-grid-addons.js:9702:20)
    at CustomDragLayer.render (react-data-grid-addons.js:9745:31)
    at finishClassComponent (react-dom.development.js:18407:31)
    at updateClassComponent (react-dom.development.js:18360:24)
    at beginWork$1 (react-dom.development.js:20108:16)
    at HTMLUnknownElement.callCallback (react-dom.development.js:362:14)
  • Another crash while playing on date filter.

MapStore.2.mp4

StandardRouter.jsx:88 TypeError: Cannot destructure 'state.filters' as it is undefined.
    at rulesmanager (rulesmanager.js:161:15)
    at combination (combineReducers.js:126:29)
    at reduce (StateUtils.js:256:14)
    at standardRootReducerFunc (defaultOptions.js:79:8)
    at rootReducer (StandardStore.js:90:12)

geosolutions-it#10924: Disabling GeoFence Rules

Description:
- fix bug of change date filter manually
- add tooltip for date fields in insert new rule or date filter in rules manager
- fix the crashing app issue of remove add rules in the grid
- add translations
@allyoucanmap
Copy link
Copy Markdown
Contributor

@tdipisa @offtherailz after the changes and clean up of JSDoc (#10814) that includes the automated script to list plugins now only plugins will checked and go in the plugins doc. The RulesManager is a page and not a plugin so afterward I moved it in the framework doc (#10877) so in the new doc for this page is here https://dev-mapstore.geosolutionsgroup.com/mapstore/docs/api/framework#pages.RulesManager. If we want to move it again inside the plugins doc we need to improve a bit the jsDocUtils.js to include pages automatically inside the plugins documentation

@offtherailz
Copy link
Copy Markdown
Member

So @mahmoudadel54 for the moment linking (with a relative path) the plugin to the page doc is enough. Something like: "This Plugin requires the configuration of the rules-manager page (link) to be activated. "

Copy link
Copy Markdown
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

LGTM. I didn't tested on GeoServer integrated version, but it can be tested as well as deployed on dev.
There is only 1 bug I noticed that have to be fixed ON BACKEND. @etj

  • There is a bug on filtering checkboxes. It happens only on IP and Date columns. Here in the video I show the IP's one, but the same can be done on date column
bug_rule_cb.mp4

The backend replies on dateAny params true or false in the same way, but for ipAddressAny or dateAny = true, it doesn't return any value. For the other parameters (e.g. requestAny = true) it simply ignores the param and works by returning all the values (because we don't have the corrispondent parameter to check).

image

basically this do not return anything:

http://localhost:8081/geofence/rest/rules/count?dateAny=true&layerAny=true&ipAddressAny=true&requestAny=true

and this returns everything

http://localhost:8081/geofence/rest/rules/count?layerAny=true&requestAny=true

That is unconsistent.

  • the date filter without dateAny do not have any difference. In this video you can see that I should be able to search "all dates that match the criteria" vs search by criteria. But both the request look only to work to match the criteria.
MapStore.3.mp4

@tdipisa
Copy link
Copy Markdown
Member

tdipisa commented Mar 24, 2025

@etj we need you to double check the above and address it.

@etj
Copy link
Copy Markdown
Member

etj commented Mar 25, 2025

I will perform more test at the end of this week, anyway the "any" boolean does not work the way you think:

  • If you don't set a filter for a given field, no "WHERE" clause will be added on such field
  • When you do set a filter on a field, there will be a WHERE clause with field=value
  • When you set a filter on a field, setting the fieldAny=true, there will be the WHERE clause field=value OR field is null, in order to retrieve also rules which impose no constraints on such field.

@offtherailz
Copy link
Copy Markdown
Member

@etj I was reading the actual tooltips for the checkbox to deduce functionality. In fact I think that while for the other rules this can be the same, in this case setting the velue Any to true has not the same meaning (because of range nature of these fields).
Maybe we have to change the tooltip, hide the checkboxes for these fields, or properly implement the functionality on BE.
@tdipisa let's check this point asap.

  • image

  • image

@etj
Copy link
Copy Markdown
Member

etj commented Mar 26, 2025

@offtherailz pls report

  • the list of rules you have in the db
  • the filtering you are requesting
  • what geofence returns

@offtherailz
Copy link
Copy Markdown
Member

offtherailz commented Mar 28, 2025

Ok, a mix of unclear behavior, not clear API pre-existing bugs generated the misundestanding.
Here I explain tha main points.

Api behavior

For any field/flagAny ( associated to the checkbox in inverse logic) The API has this behaviour:

  1. Value is missing
    1. flag not present -> no filtering
    2. flagAny=false -> no filtering
    3. flagAny=true -> All the rules that has null in the given field
  2. Value is present
    1. flag not present -> search all fields that match the rules (including nulls)
    2. flagAny=false -> all rules that match only because of the specific field (excluding nulls)
    3. flagAny=true -> earch all fields that match the rules (including nulls) as (2.i)

MapStore bug

The client has a pre-existing bug that, initially do not send the flag, then when checked sends the flag to false (inverted logic) and then when unchecked sends the flag to true. There is no way to reset the flag.

Instead, the checkbox should send only the flag to reproduce the conditions:

  • With no value, the flag should not be send (actually it happens only on startup, after check/uncheck the value is always sent)
  • When value is set:
    • When checked, the flag should be send with value false
    • When unchecked the flag should be sent with value true.
  • In order to better clarify the behavior and avoid future misunderstanding, I'd also modify the tooltips as indicated at the end of this comment

This has to be fixed, maybe in a dedicated issue/PR.

Expected functionality

As a user I'd expect the functionality to search by content or by match. Checking by "all the rules thah match but not because of null" is hard to understand for a user, but it is what is offered by the current API.

We can not change or enhance GeoFence, so I'd at least better indicate the filtering condition.

The current tooltip tells to check to "filter by selected value" that is a little generic.
image

I'd suggest the following tooltips:

  • When checked (flagAny=false) → "Currently showing only rules that match the provided value. Uncheck to also include rules where the field value is missing."

  • When unchecked (flagAny=true) → "Currently including all rules that match, even if different from the given value. Check to filter only by this field value."

@tdipisa
Copy link
Copy Markdown
Member

tdipisa commented Mar 28, 2025

@offtherailz and @etj thank you so much for clarifying the above.

@mahmoudadel54 we need you now to provide:

  • The fix to the filter check logic
  • The tooltip improvement

see above for more details.

…ltips of any field checkbox in rule managers, handles cases of check/uncheck

Description:
- handles these cases in check/uncheck any field in rules manager grid:
* with no value, the flag should not be send
* When value is set:
   - When checked, the flag should be send with value false
   - When unchecked the flag should be sent with value true.
- add unit tests
- edit tooltips + translations for check/uncheck in rules manager
Copy link
Copy Markdown
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

@tdipisa I had to fix tooltips initially achieved because translation where a lot misleading.
Can you take a look to have a second opinion. Thank you.

(I also fixed tootip position that was making the tooltip go out of screen for some fields.

@tdipisa tdipisa self-requested a review April 3, 2025 07:43
Copy link
Copy Markdown
Member

@tdipisa tdipisa left a comment

Choose a reason for hiding this comment

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

@offtherailz I would write like this (see my change). If you agreed, please translate the same for other languages.

Co-authored-by: Tobia Di Pisa <tobia.dipisa@geosolutionsgroup.com>
@tdipisa tdipisa merged commit 10f2df5 into geosolutions-it:master Apr 4, 2025
6 checks passed
@tdipisa tdipisa added BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch and removed BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch labels Apr 4, 2025
@offtherailz
Copy link
Copy Markdown
Member

offtherailz commented Apr 4, 2025

@ElenaGallo, could you please test this on DEV ? Thank you
Ideally we should test:

  • Standalone new version (nightly)
  • Embedded new version (nightly)
  • Standalone old version (I suppose this tool needs to work with current and old versions of geoserver/geofence too)
  • Embedded old version (I suppose this tool needs to work with current and old versions of geoserver/geofence too)

Actually on dev we can test only embedded old version.
During dev we tested the Standalone new version.

Let's check if the functionalitries break something (e.g. adding an ip rule or a date rule, trying to filter by ip or by date).
Try to save edit and filter also by other fields (e.g. user or layer), to verify the functionalities are preserved.

@tdipisa tdipisa modified the milestones: 2025.01.00, 2025.02.00 Apr 10, 2025
@ElenaGallo
Copy link
Copy Markdown
Contributor

@mahmoudadel54 @tdipisa The Date functionality works, but the Date filter is not clear to me.
If I enter the interval >2025-11-18 & <2025-11-19 in the rule, when I filter for the date 2025-11-18 in the filter box, the rule with >2025-11-18 & <2025-11-19 does not appear in the list.
In this case, shouldn’t the date 2025-11-18 be included?

date

@mahmoudadel54
Copy link
Copy Markdown
Contributor Author

@mahmoudadel54 @tdipisa The Date functionality works, but the Date filter is not clear to me. If I enter the interval >2025-11-18 & <2025-11-19 in the rule, when I filter for the date 2025-11-18 in the filter box, the rule with >2025-11-18 & <2025-11-19 does not appear in the list. In this case, shouldn’t the date 2025-11-18 be included?

The docs here: https://docs.geoserver.org/latest/en/user/extensions/geofence-server/rest.html#filter-parameters
mentioned:

image

So I think the filtered date you entered not included.
@tdipisa can confirm if it is right or not.

@tdipisa
Copy link
Copy Markdown
Member

tdipisa commented Dec 1, 2025

@mahmoudadel54 exactly. @ElenaGallo only single date value is allowed in the filter. Please check and let me know asap if we can mark the issue as Accepted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

New Feature used for new functionalities

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add filter by IP for GeoFence rules Disabling GeoFence Rules

7 participants