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

LDIF's DefaultAttributeValidationPolicy doesn't comply with RFC2849 - it doesn't allow UTF8 characters in attribute values #492

Closed
naorbar opened this issue Sep 5, 2018 · 5 comments

Comments

@naorbar
Copy link

naorbar commented Sep 5, 2018

The given LDIF doesn't comply with RFC2849: "LDAP Data Interchange Format (LDIF) - Technical Specification"
For more details see:
https://en.wikipedia.org/wiki/LDAP_Data_Interchange_Format
https://tools.ietf.org/html/rfc2849
For instance, it throws an InvalidAttributeFormatException (line 318) when the LDIF contains an attribute such as this one:
company: Østfold Akershus
To workaround it, I could do one of the following:

  1. Implement my own Utf8AttributeValidationPolicy which looks very similar to the DefaultAttributeValidationPolicy and use it in my LdifParser:
    e.g.
    ldifParser.setAttributeValidationPolicy(new UTF8AttributeValidationPolicy());
  2. Extend the DefaultAttributeValidationPolicy class

Option (2) should be easier and follow the open-closed OOD principal, but unfortunately in this case, the class contains only private members which can not be used in the inherited class.

So please fix the following:

  1. Please add support for UTF-8 characters in DefaultAttributeValidationPolicy to comply with RFC2849.
  2. Please change the members' and methods' modifiers in DefaultAttributeValidationPolicy to protected/public, so everyone will be able to extend this class.

Thank you,
Naor

@rwinch
Copy link
Member

rwinch commented Sep 5, 2018

It seems quite reasonable to support UTF-8 characters. Would you be interested in submitting a PR for this?

If we provide support for UTF-8 out of the box, I don't see a need for changing the visibility of the members/methods. I'd prefer to wait to have an explicit reason for doing this. Can you think of a concrete reason that we need to do this now? If not, then I don't think this is something we will do. If so, it might be worth considering additional improvements.

@rwinch rwinch self-assigned this Sep 5, 2018
@naorbar
Copy link
Author

naorbar commented Sep 6, 2018

Rob, thank you for your quick response.

Changing the visibility of the members and methods is an easy and intuitive solution:
suppose I just got a LDIF from a Asian/European customer which contains spacial characters... in that case I will probably want to extend DefaultAttributeValidationPolicy - use the String, Base64 and URL validations and add my own specific validations.
Another example: suppose I want to enforce a more strict LDIF format for one of my customers - best option again is to extend DefaultAttributeValidationPolicy, rather than implementing the whole class from scratch.

I'd be happy to submit a pull request, please let me know which fix fits here (after considering my examples above): (1) or (2), or both...

Naor

@rwinch
Copy link
Member

rwinch commented Sep 6, 2018

I'd prefer to wait on exposing DefaultAttributeValidationPolicy members until we have a real use case to solve. Breaking encapsulation makes the code harder to maintain and breaking it before we have real solutions often leads to the wrong solution.

I'd be glad to accept a PR for UTF-8 support as this is something that is concrete.

naorbar pushed a commit to naorbar/spring-ldap that referenced this issue Sep 12, 2018
…tes values for LDIF's DefaultAttributeValidationPolicy in order to comply with RFC2849
@naorbar
Copy link
Author

naorbar commented Sep 12, 2018

Thank you, Rob.
PR #493
Naor

rwinch pushed a commit that referenced this issue Sep 13, 2018
…LDIF's DefaultAttributeValidationPolicy in order to comply with RFC2849
@rwinch rwinch added status: waiting-for-feedback We need additional information before we can continue and removed waiting-for-feedback labels May 4, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Sep 30, 2024

Closed in 177b4bb

@jzheaux jzheaux closed this as completed Sep 30, 2024
@jzheaux jzheaux added type: enhancement in: core and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants