search

SAML-Toolkits / wordpress-saml

OneLogin SAML plugin for Wordpress
MIT License
65 stars 74 forks source link

add ability to expose attributes that come from saml via a WordPress … #59

Closed matgargano closed 6 years ago

matgargano commented 6 years ago

… action

schuhwerk commented 6 years ago

Great! Had the same issue and used the same (intermediate) solution. Seems cleaner to me to leave the "get_current_user()" and "get_current_user_id()" here though. And why not move the do_action a line further up?

if (is_a($user_id, 'WP_Error')) {
    $error = $user_id->get_error_messages();
    echo implode('<br>', $error);
    exit();
} else if ($user_id) {
    wp_set_current_user($user_id);
    wp_set_auth_cookie($user_id);
    setcookie('saml_login', 1, time() + YEAR_IN_SECONDS, SITECOOKIEPATH );
    do_action('onelogin_saml_after_login_success', $attrs);
}

Wouldn't that mean less errorhandling? Like checking for an empty $user_id?

matgargano commented 6 years ago

@tivus I'm not 100% sure I get where you mean. I am doing it nearly exactly where you are, just outside of the control structure. Note that the WP plugin and this repo are significantly different....

schuhwerk commented 6 years ago

@matgargano Sorry, I referenced an old version. Tldr: don't mind my comment :) I looked at the code and both wp_insert_user and wp_update_user can affect the $user_id to become empty, 0, false,... But as both functions never return any of those, it doesn't really matter, if it is within or outside the control structure.

matgargano commented 6 years ago

sweet -- hopefully this gets merged!