Fix error sending email with capitalization to ApigeeX Org

[shishir-intelli]

Closes #730

As ApigeeX Org is not accepting capitalized email address, we are forcing lowercase email addresses for Apigee-X Org to prevent error.

[mxr576]

Sorry, but I believe this should be a new field level constraint which would run whenever and wherever $user->validate() or $userl->mail->validate() is called. This would be the standard Drupal/Symfony way or ensuring data consistency.

As a last result, a sanity check could still be executed in apigee_edge_user_presave() to ensure the above described field level validation was actually validated before entity save - because we all know that some IDP integration modules loves to bypass validating the user entity before they save it - and if an email still contains uppercase characters a LogicException (or a subclass of it) could be thrown to prevent user creation. (Which is still better than spending hours on figuring our why they is a Drupal user in the system without a developer account on the Apigee side.)

[sagar-lcm]

I know of a case where the client's LDP was using upper-case characters in every user's email address, which apparently was getting synced with Apigee. The client was reluctant to change the LDP configurations so wrote a hook to convert the new user's email address to lowercase and also an update hook to convert the existing user's email address to lowercase which in turn syncs the developer account Apigee as well.

[mxr576]

I have security concerns regarding this approach because I am quite sure that somewhere in the user create/update process it is checked whether the new/changed email address of Drupal user is already belongs to a developer account on Apigee, BUT if that runs before this code then somebody could easily take over a developer account of somebody else by using a uppercase version of the email address.

[shishir-intelli]

Thank you @mxr576 for reviewing this PR.

Do you mean to say, that instead of forcefully converting the email address to lowercase we should put a constraint on the email field so the user gets an error message while user create/update if capitalized email address found for Apigee-X Org.

[mxr576]

Yes, exactly - And developer sync is also going to fail (it already works like this) when entity validation fails.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.85%. Comparing base (afeb21e) to head (b1e4fa5).

❗ Current head b1e4fa5 differs from pull request most recent head ee49b56. Consider uploading reports for the commit ee49b56 to get more accurate results

Additional details and impacted files

@@            Coverage Diff            @@
##                2.x     #756   +/-   ##
=========================================
  Coverage     44.85%   44.85%           
  Complexity     3047     3047           
=========================================
  Files           345      345           
  Lines         11178    11178           
=========================================
  Hits           5014     5014           
  Misses         6164     6164           

[dpagini]

Just wanted to check in on this issue and see if there's any further movement on this...?

Looking at this PR, I would not be expecting this module to control how I am storing a user email in my Drupal site. I guess, to me, the fix I would expect is simply that, when sending an email over to Apigee, that it's transformed to lowercase at that time. Leave it stored in whatever case it's stored in on Drupal... Is that not viable?

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[dpagini]

Hi Apigee team - Is there any direction on this pull request? This has been open for more than a year now, with little discussion. There are some valid discussions that are still open here. What are the next steps to move this issue forward?

[shishir-intelli]

Hi @dpagini ,
Team is reviewing this PR for any changes and approve it.

[kedarkhaire]

Hi @shishir-intelli I checked with other cases also,
with Okta SAML login also, I checked with camelcase, It works fine and the email is converted to lowercase with message.

[kedarkhaire]

LGTM

[boobaa]

Please note that this approach goes against the SMTP standard: https://datatracker.ietf.org/doc/html/rfc5321#section-2.4 clearly says:

The local-part of a mailbox MUST BE treated as case sensitive.

If this gets merged, all kind of breaks are expected, not only in Drupal, but also in receiving emails, etc.

[kedarkhaire]

Hi @boobaa

We are planning to add the following to avoid the duplicate developers with same email ids.

During new user creation - Add a duplicate email id check (Includes a check with lists of existing developers in that X org)
When a existing user is logged in - Add a duplicate email id check + Add a status report message of system requirements + Restrict users to show the API keys.

If you have any other additional to-dos, we are open for a discussion over here.

[mxr576]

Do not forget about email changing in the Drupal UI, that should be also handled... But not even Drupal core handles that (still) https://www.drupal.org/node/85494

Even if think this auto-lowercasing is a bad idea... Let me try to imagine how this could work securely... Before the lowercased email is saved, the ownership must be confirmed via opening a verification link from the mailbox or similar.
Developers are going to have plenty ways to shoot themselves on foot if one system uses upperscase/mixed case emails the other silently converts them to lowercase, just imagine that email stored in the idp gets used for loading a suppose to be existing developer on Apigee.

[dpagini]

I'm not sure I agree with this approach, either.
I don't think we should be changing the case of an email on the Drupal side. This is very heavy handed.
Maybe this is not something that should even be fixed in the module, but instead on the Apigee X Org end to accept mixed case email addresses...? Maybe that's a bigger change, but based on some of the links shared above, it seems like this is a problem with the API endpoint, and not with the integration.

[shishir-intelli]

This issue has been resolved here #1050