Skip to content

shibboleth_authenticate_user: cleanup#38

Merged
michaelryanmcneill merged 2 commits intomichaelryanmcneill:masterfrom
jrchamp:cleanup
May 15, 2018
Merged

shibboleth_authenticate_user: cleanup#38
michaelryanmcneill merged 2 commits intomichaelryanmcneill:masterfrom
jrchamp:cleanup

Conversation

@jrchamp
Copy link
Collaborator

@jrchamp jrchamp commented Feb 16, 2018

A couple small cleanup items I noticed while working on the other issue:

  • Remove redundant update_user_meta for shibboleth_account
    • Already set on create by shibboleth_create_new_user() and checked using get_user_meta() for existing accounts
  • Reorder by-email combine checks to match by-username checks

…der by-email combine checks to match by-username checks
@jrchamp
Copy link
Collaborator Author

jrchamp commented Feb 21, 2018

Here's an idea that I think makes the "account combine" code more clear that if shibboleth_account isn't set, we're either going to set it or throw a WP_Error.

// look up existing account by username, with email as a fallback
$user_by = 'username';
$user = get_user_by( 'login', $username );
if ( ! $user ) {
        $user_by = 'email';
        $user = get_user_by( 'email', $email );
}

// if this account is not a Shibboleth account, then do account combine (if allowed)
if ( is_object( $user ) && $user->ID && ! get_user_meta( $user->ID, 'shibboleth_account' ) ) {
        $do_account_combine = false;
        if ( $user_by === 'username' && ( $auto_combine_accounts === 'allow' || $manually_combine_accounts === 'allow' ) ) {
                $do_account_combine = true;
        } elseif ( $auto_combine_accounts === 'bypass' || $manually_combine_accounts === 'bypass' ) {
                $do_account_combine = true;
        }

        if ( $do_account_combine ) {
                update_user_meta( $user->ID, 'shibboleth_account', true );
        } elseif ( $user_by === 'username' ) {
                return new WP_Error( 'invalid_username', __( 'An account already exists with this username.', 'shibboleth' ) );
        } else {
                return new WP_Error( 'invalid_email', __( 'An account already exists with this email.', 'shibboleth' ) );
        }
}

If you like this, I can add it to this PR.

@michaelryanmcneill
Copy link
Owner

Yeah, I like that @jrchamp. Go ahead and add that to the PR and I'll get it merged in.

@jrchamp
Copy link
Collaborator Author

jrchamp commented Mar 16, 2018

Ok, I've added the account combine unification commit to the PR.

@michaelryanmcneill michaelryanmcneill merged commit 99c518b into michaelryanmcneill:master May 15, 2018
@jrchamp jrchamp deleted the cleanup branch August 7, 2020 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants