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

LDAP: Improve username case sensitivity support (assist #4821). #3078

Merged
merged 20 commits into from Oct 9, 2014

Conversation

bpindelski
Copy link

See https://trac.openmicroscopy.org.uk/ome/ticket/4821.
This is an initial attempt at allowing clients to use a mixed-case username when logging in (this includes CLI, Web and Insight). The changes started off as LDAP-only, but due to the tight coupling between the classes taking part in the user authentication process, the commits had to cover all scenarios (LDAP and non-LDAP).

Warning!
This PR will require the sysadmin to manually lower-case all usernames before setting omero.security.ignore_case to true. This PR doesn't handle experimenter login clashes during such a procedure.
This PR also doesn't solve the omero.ldap.user_lookup_attributes requirement, which could come in a subsequent PR.

To test:

  • use a server that has LDAP enabled and some non-LDAP users too,
  • set omero.security.ignore_case to true and update all entries in the experimenter table (omename column) so that they are lower-case, e.g.
BEGIN;
update experimenter set omename = lower(omename);
COMMIT;
  • restart the server,
  • try logging in with mixed case login names (e.g. uSeR-1, rOOt). Logins should work as before.

@jburel jburel added the develop label Sep 30, 2014
@atarkowska
Copy link
Member

Tested with Virtual Microscope database. All seems to work OK. There were no username conflicts as we use only LDAP

@bpindelski
Copy link
Author

@joshmoore or @mtbc: If you see any glaring mistakes in the code, I'd appreciate any comments. Thanks!

<constructor-arg index="7" value=""/>
</bean>

<bean id="testIgnoreCase" class="java.lang.Boolean">
Copy link
Member

Choose a reason for hiding this comment

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

not sure why this bean is more indented

@mtbc
Copy link
Member

mtbc commented Oct 1, 2014

No obvious horrors at least. (-:

@bpindelski
Copy link
Author

@mtbc Thanks, errors now corrected.

@mtbc
Copy link
Member

mtbc commented Oct 2, 2014

Thank you, good to merge.

@bpindelski
Copy link
Author

Final question before merging (probably to @joshmoore): How do we want to expose this? Is documentation required (in a similar "experimental" form as the in-place doc)?

@joshmoore
Copy link
Member

@bpindelski : I would think this falls under the "DEVELOPMENT_SETTINGS" heading as we've done with Web, but we don't yet have tools for hiding those (/cc @sbesson). Perhaps for the moment, just add a clear warning and/or link to the proper FAQ in etc/omero.properties so no one tries to use it without knowing the steps they need to take.

@sbesson
Copy link
Member

sbesson commented Oct 6, 2014

Is it worth blacklisting explicitly from the auto-generated configuration page (https://github.com/openmicroscopy/openmicroscopy/blob/develop/components/tools/OmeroPy/src/omero/install/config_parser.py#L128) ?

@joshmoore
Copy link
Member

If you have a suggestion on how to do that, yes.

@bpindelski
Copy link
Author

@sbesson, @joshmoore I've updated the text in omero.properties. Please let me know if you want me to add anything to our LDAP docs, as that is what I meant by "exposing". I can try and "hide" the setting once @sbesson points me towards a way of doing that.

@sbesson
Copy link
Member

sbesson commented Oct 7, 2014

So far, I think we are limited to simply adding omero.security.ignore_case to the BLACK_LIST tuple. This should be enough to prevent this configuration property from appearing in the doc.

@bpindelski
Copy link
Author

@sbesson Done.

@sbesson
Copy link
Member

sbesson commented Oct 8, 2014

Thanks. This is enough for omero.security.ignore to be stripped from snoopycrimecop/ome-documentation@2242f41. As suggested by @joshmoore we may want to move toward a more useful ## EXPERIMENTAL/DEVELOPMENT markup but this is outside the scope of this PR and I can start working on it once this is merged.

@bpindelski
Copy link
Author

@sbesson Thanks for checking. The diff from the autogen job highlighted one shortcoming in my previous commit - I didn't change the comment marker to be ##, which I have now corrected. In the next build, the autogen won't have the stray comment included.

@atarkowska
Copy link
Member

all good for me

@joshmoore
Copy link
Member

http://ci.openmicroscopy.org/job/OMERO-5.1-merge-docs-autogen/72/ certainly doesn't contain the property. 👍

joshmoore added a commit that referenced this pull request Oct 9, 2014
LDAP: Improve username case sensitivity support (assist #4821).
@joshmoore joshmoore merged commit 77d11b6 into ome:develop Oct 9, 2014
@bpindelski bpindelski deleted the 4821_ldap_redux branch October 9, 2014 18:56
@bpindelski
Copy link
Author

--no-rebase

@sbesson sbesson modified the milestones: 5.1.0-m1, 5.1.0-m2 Oct 14, 2014
zeb added a commit to imagopole/omero-auth-ppms that referenced this pull request May 26, 2015
All external password provider lookups are case-sensitive and
non-configurable by default

See: ome/openmicroscopy#3078
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants