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

#265 LDAP groups and roles concepts are mixed #266

Merged
merged 2 commits into from
Apr 15, 2022
Merged

Conversation

etj
Copy link
Member

@etj etj commented Apr 14, 2022

Proposed changes:

  • Separating mappers for roles and concepts
  • Added configuration to prevent unmapped authorities to be stored
  • Keep older bean definition in place for back compatibility
  • Improved logging

After upgrading, if you don't change your xml configuration, you may get this error in the logfile:
AuthoritiesMapper is deprecated, please set roleMapper and groupMapper separately

This means you have, in the geostoreLdapProvider bean definition, the setter for the property authoritiesMapper (which is now deprecated).
In order to maintain the old behaviour, the authoritiesMapper will set the new 2 properties, roleMapper and groupMapper, that can now be separately defined.

Once the fix is applied, you may want to:

In geostoreLdapProvider, change the entry:

   <property name="authoritiesMapper" ref="rolesMapper"/>

to

   <property name="roleMapper" ref="rolesMapper"/>

In the rolesMapper bean, add the property:

   <property name="dropUnmapped" value="true" />

@tdipisa tdipisa requested a review from taba90 April 14, 2022 13:00
@nmco
Copy link
Collaborator

nmco commented Apr 15, 2022

A couple of things are not clear here:

  • For which client is this? The label on THE ISSUE?
  • The testing doesn't seem to be complete enough.
  • Why is this PR not targeting the master branch?

@simboss
Copy link
Member

simboss commented Apr 15, 2022

  • C047-CLEVELAND-METROPARKS-2021-SUPPORT
  • this is a priority fix for a production issue for which we can't wait weeks to deliver so it is targeting directly the branch they are using since we are delivering a patch quickly but we are sharing this with the team to properly clean, test and port on all active branches later on

@etj please, provide more info next time ( see our yesterday discussion on delegation)

@ale-cristofori
Copy link

ale-cristofori commented Apr 15, 2022

A couple of things are not clear here:

  • For which client is this? The label on THE ISSUE?
  • The testing doesn't seem to be complete enough.

here more info https://github.com/geosolutions-it/MapStore2-C047/issues/28

Why is this PR not targeting the master branch?

thanks Notes for developers, Cleveland are using geostore version 1.7.1, the PR associated to this issue has to be forward ported to main branch.

@nmco
Copy link
Collaborator

nmco commented Apr 15, 2022

Since this is super urgent I would recommend that you do as I told you on the chat @ale-cristofor, merge it a deploy it where you need it, then open an issue describing that we need to handle this properly:

  • Bring this to master
  • Add the necessary tests

@nmco nmco merged commit 647e3c8 into 1.7.x Apr 15, 2022
@ale-cristofori
Copy link

the geostore jar file in this MapStore down stream project (located at C:\Users\acristofori\Development\MapStore2-C047\web\target\mapstore\WEB-INF\lib) are pointing to 1.7-SNAPSHOT in the pom.xml files in the project the target version is 1.7.1. I tried to build jars with version 1.7-SNAPSHOT but we are running into errors about geostore not being able to connect to data storage and retrieve resources (maps, geostories, etc) @taba90 we might need your help to build and deploy.

@nmco
Copy link
Collaborator

nmco commented Apr 15, 2022

@taba90 is already out for today @ale-cristofori.

@taba90 let's have a look at this Tuesday morning, if needed we move the Vlaaderen call with Bert.

taba90 pushed a commit to taba90/geostore that referenced this pull request May 9, 2022
taba90 pushed a commit to taba90/geostore that referenced this pull request May 9, 2022
taba90 added a commit that referenced this pull request May 9, 2022
* Merge pull request #266 from geosolutions-it/265_17x_ldap_roles

* Tests for LDAP groups and roles concepts are mixed (#272)

Co-authored-by: Nuno Oliveira <[email protected]>
taba90 added a commit that referenced this pull request May 9, 2022
* Merge pull request #266 from geosolutions-it/265_17x_ldap_roles

* Tests for LDAP groups and roles concepts are mixed (#272)

Co-authored-by: Nuno Oliveira <[email protected]>
@afabiani afabiani deleted the 265_17x_ldap_roles branch April 3, 2024 15:46
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.

4 participants