Navigation Menu

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

ldapadmin - administrators can modify users' uid #1109

Merged
merged 4 commits into from Nov 3, 2015

Conversation

pmauduit
Copy link
Member

@pmauduit pmauduit commented Nov 2, 2015

Evolution funded by CIGALsace (https://github.com/camptocamp/georchestra-cigalsace-configuration/issues/531).

Enables the ability for LDAP administrators (members of MOD_LDAPADMIN) to modify the user id (uid) of the users LDAP entries.

The uid field is not like the others attributes and need extra work to implement the ability to modify it, since it is part of the DN qualification ; it requires to "move" the LDAP entry, which is not the same as updating a regular attribute.

It also requires to get the users groups and update the member entry (server-side but also client-side in the js objects).

Tests:

  • Utests + tests against a real ldap adapted / OK
  • Runtime tests OK
  • No added tests though, might deserve a one, at least onto a regular LDAP.
  • Adding the "Warn by mail" feature

Evolution funded by CIGALsace
(https://github.com/camptocamp/georchestra-cigalsace-configuration/issues/531)

Enables the ability for LDAP administrators (members of MOD_LDAPADMIN)
to modify the user id (uid) of the users LDAP entries.

The uid field is not like the others attributes and need extra work to
implement the ability to modify it, since it is part of the DN
qualification ; it requires to "move" the LDAP entry, which is not the
same as updating a regular attribute.

It also requires to get the users groups and update the member entry.

Tests: Utests + tests against a real ldap adapted / OK
Runtime tests OK
@pmauduit
Copy link
Member Author

pmauduit commented Nov 2, 2015

Please do not merge yet

- Adding synchronized on CRUD operations from AccountDaoImpl to avoid
  race conditions (Note: might not the best way to do, but this is what
  was already done onto GroupDao).

- Adding a test to cover modifyUser(Account, Account), tested against a
  real ldap.
@pmauduit
Copy link
Member Author

pmauduit commented Nov 2, 2015

Should be good to go now.

@@ -75,5 +84,36 @@ public void testBlankFields_issues_1086_1096() throws Exception {
assertTrue("Found a 'o' attribute, expeceted none", noOrgAnymore);

}

@Test
public void testUpdateAcountAccount() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

testUpdateAcountAccount ?

Copy link
Member Author

Choose a reason for hiding this comment

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

because the method signature is update(Account old, Account new), and I did not have any imagination for a test name :)

@fvanderbiest
Copy link
Member

OK, LGTM, thanks Pierre, please merge.

@pmauduit
Copy link
Member Author

pmauduit commented Nov 3, 2015

Missing the "send a mail feature", I will work on it before merging.

pmauduit added a commit to georchestra/template that referenced this pull request Nov 3, 2015
Tests:
- Mainly tested at runtime, expected mail provided to the
mailserver
- Testsuite OK
@pmauduit
Copy link
Member Author

pmauduit commented Nov 3, 2015

good to go.

@fvanderbiest
Copy link
Member

LGTM, please merge !

pmauduit added a commit that referenced this pull request Nov 3, 2015
ldapadmin - administrators can modify users' uid
@pmauduit pmauduit merged commit 363f28a into master Nov 3, 2015
@fvanderbiest
Copy link
Member

Looks like there's a small side-effect ...

org.springframework.ldap.NameNotFoundException: [LDAP: error code 32 - No Such Object]; nested exception is javax.naming.NameNotFoundException: [LDAP: error code 32 - No Such Object]; remaining name 'uid=toto,ou=users'
    at org.springframework.ldap.support.LdapUtils.convertLdapException(LdapUtils.java:183)
    at org.springframework.ldap.core.LdapTemplate.executeWithContext(LdapTemplate.java:820)
    at org.springframework.ldap.core.LdapTemplate.executeReadOnly(LdapTemplate.java:803)
    at org.springframework.ldap.core.LdapTemplate.lookup(LdapTemplate.java:935)
    at org.georchestra.ldapadmin.ds.AccountDaoImpl.findByUID(AccountDaoImpl.java:311)
    at org.georchestra.ldapadmin.ds.AccountDaoImpl.insert(AccountDaoImpl.java:104)
    at org.georchestra.ldapadmin.ws.newaccount.NewAccountFormController.create(NewAccountFormController.java:153)

@@ -61,6 +61,7 @@ shared.download_form.pdfurl=
shared.ldapadmin.contextpath=/ldapadmin
shared.ldapadmin.db=georchestra
shared.ldapadmin.jdbcurl=jdbc:postgresql://@shared.psql.host@:@shared.psql.port@/@shared.ldapadmin.db@
shared.ldapadmin.warnifuidmodified=true
Copy link
Member

Choose a reason for hiding this comment

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

My mistake, this is not a real shared maven filter. It only belongs to ldapadmin.
=> should not be set in config/shared.maven.filters but in build_support/GenerateConfig.groovy

@fvanderbiest fvanderbiest deleted the ldapadmin-admin-can-rename-uid branch January 27, 2016 13:10
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.

None yet

2 participants