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

Mail use fullname #71

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Jan 5, 2017

I wrote this because I wanted my emails to look more pretty by displaying the fullname of a user instead of the username/login. Instead of "jsmith92" you can also use "Jones Smith" or just "Jones". Default option is still the login name.

David Lang and others added 5 commits December 14, 2016 16:08
Added config option to get mail adress from LDAP for resetbytoken. Di…
Append mail_signature to every Mail that is sent by the system. Defau…
i.e. "fullname" -> Hello Jones Smith
     "givenname" -> Hello Jones
     "login" -> Hello jsmith
@ghost
Copy link
Author

ghost commented Jan 5, 2017

Found a bug, will resolve it by tomorrow.

@coudot coudot added this to the 1.1 milestone Jan 6, 2017
@coudot
Copy link
Member

coudot commented Jan 6, 2017

Thanks for the PR, will have a look later.

@ghost
Copy link
Author

ghost commented Mar 21, 2017

What is the status here?

@plewin
Copy link
Member

plewin commented Mar 21, 2017

I believe SSP should provide a way to override lang files without touching the provided files in /lang/* to offer more flexibility.

For example, by including a new file "conf/lang_overrides.inc.php" after the require_once("lang/$lang.inc.php");
https://github.com/ltb-project/self-service-password/blob/master/index.php#L47

This file would replace {login} by {firstname} in email templates, $messages['resetmessage'], $messages['changemessage']

The
$data = array( "login" => $display_name_lookup[$mail_display_name], "mail" => $mail, "url" => $reset_url ) ;
would be
$data = array( "login" => $login, "mail" => $mail, "url" => $reset_url, "firstname" => ..., ..., ..) ;

@ghost
Copy link
Author

ghost commented Mar 21, 2017

@plewin thanks for your idea, this would be a much better way to deal with it.
In this case it would be great to update the whole language "backend" a little bit. The current way of dealing with translations seems a bit cumbersome to me.

@plewin
Copy link
Member

plewin commented Mar 21, 2017

@dlang9159 Sorry if it is getting a bit off topic but what changes would you like to see in the language backend ?

@ghost
Copy link
Author

ghost commented Mar 21, 2017

@plewin For instance the one you already mentioned: I'd like to alter some strings like the mail text for example add a signature ( #67 was kind of a workaround).

But the most "annoying" thing is that you have to add new language strings to all *.inc.php files manually (or did I miss something). It would be more convenient to use a look-up function that returns the (english) default string if the language doesn't support it yet.

But this is just my personal opninion and it is open to discussion. I am aware that this leads to more complexity.

Maybe I should add an issue where we can discuss this in more depth.

@coudot
Copy link
Member

coudot commented Mar 21, 2017

Sorry for delay, I still need to check the code and merge it

@coudot
Copy link
Member

coudot commented Mar 24, 2017

Changing how we manage language files will not solve completely this issue, as we need to search in LDAP directory the attribute we want to put in the message.

So we need at least a configuration parameter that can be an array and set all attributes we want to get from user entry.

Then we can indeed have an easy way to override lang messages. I've done that in White Pages:

if (file_exists("../conf/$lang.inc.php")) {
    require_once("../conf/$lang.inc.php");
}

It allows to define a lang file in conf directory that will override or add messages.

What do you think?

@ghost
Copy link
Author

ghost commented Mar 24, 2017

This looks ok for me. Of course we must define what attributes we want to get.
I suggest: uid/login, givenName, fullname.
Any other attributes? How do we handle attributes with multiple values?

If we agree on your suggestion for the language file I will alter my PR to support this feature.

@coudot
Copy link
Member

coudot commented Mar 24, 2017

@dlang9159 Attributes will be configured, but we can indeed have a default value with these ones, for example:

$ldap_attributes = array('uid', 'cn', 'sn', 'givenname');

I will open a new issue for the language file override.

@plewin
Copy link
Member

plewin commented Mar 24, 2017

I believe the default should be an empty array

# Fetch extra attributes for the email templates
# Example : $fetch_extra_attributes_for_mail = array('uid', 'cn', 'sn', 'givenname');
# Template will be able to use {uid} {cn} {sn} {givenname}
$fetch_extra_attributes_for_mail = array();

If there is no extra attributes to fetch, nothing will be fetched.

Also I think there is a bit too much of code duplication in this PR.

@ghost
Copy link
Author

ghost commented Mar 24, 2017

@plewin you are totally right with code duplication, I will move most of it to lib/functions.inc.php

@gino909
Copy link

gino909 commented Jul 21, 2017

Hi
to make this happen I have modified following scripts
pages/sendtoken.php

# Match with user submitted values
foreach ($mailValues as $mailValue) {
    if (strcasecmp($mail_attribute, "proxyAddresses") == 0) {
        $mailValue = str_ireplace("smtp:", "", $mailValue);
    }
    if (strcasecmp($mail, $mailValue) == 0) {
        $match = 1;
        # Get user name for notification
        $cnValues = ldap_get_values($ldap, $entry, $ldap_fullname_attribute);
        $username = $cnValues[0];
    }
}

...

$data = array( "username" => $username, "login" => $login, "mail" => $mail, "url" => $reset_url ) ;

pages/resetbytoken.php

# Get user email for notification
if ( $notify_on_change ) {
    $mailValues = ldap_get_values($ldap, $entry, $mail_attribute);
    if ( $mailValues["count"] > 0 ) {
        $mail = $mailValues[0];
        # Get user name for notification
        $cnValues = ldap_get_values($ldap, $entry, $ldap_fullname_attribute);
        $username = $cnValues[0];
    }
}

....

$data = array( "username" => $username, "login" => $login, "mail" => $mail, "password" => $newpassword);

pages/change.php

# Get user email for notification
if ( $notify_on_change ) {
    $mailValues = ldap_get_values($ldap, $entry, $mail_attribute);
    if ( $mailValues["count"] > 0 ) {
        $mail = $mailValues[0];
        **# Get user name for notification
        $cnValues = ldap_get_values($ldap, $entry, $ldap_fullname_attribute);
        $username = $cnValues[0];**
    }
}

...

$data = array( "username" => $username, "login" => $login, "mail" => $mail, "password" => $newpassword);

At the end change the tag {login} to {username} in your translation files.

I was not tested with Windows AD!

@coudot coudot modified the milestones: Future, 1.1 Aug 30, 2017
@coudot
Copy link
Member

coudot commented Aug 30, 2017

We must find a cleaner way to get attributes from user and use them in mails. The configuration must be generic

@JuJuMDX
Copy link
Contributor

JuJuMDX commented Feb 26, 2020

Hello,

This is an old PR for SSP 1.1 so i would like to know if it's still necessary to keep it open ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants