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

Bug: Logging out using redirect authentication does not log out the user #1147

Open
ghost opened this issue Nov 14, 2022 · 11 comments
Open

Bug: Logging out using redirect authentication does not log out the user #1147

ghost opened this issue Nov 14, 2022 · 11 comments
Labels
package: core (legacy) Related to the legacy core package package: @faustwp/core Related to the Faust Core package package: faustwp Related to the companion WordPress plugin type: feature New functionality being added

Comments

@ghost
Copy link

ghost commented Nov 14, 2022

When logging out using the redirect method of authentication, the frontend cookie WordPress authentication persists and the user remains logged in to the application. I was able to reproduce this on a fresh install.

Applicable Versions

  • @faustjs/core version: 0.15.7
  • @faustjs/react version: 0.15.7
  • @faustjs/next version: 0.15.9
  • WordPress version: 6.1

Steps To Reproduce

  1. Install my repo https://github.com/cfg-fadica/faustjs-logout-demo
  2. Connect the repo to a WordPress backend
  3. Start the server and visit localhost:3000/authpage
  4. Click log in and log in with valid credentials
  5. Once redirected back to the frontend app, click the log out link. This will show you the "unauthenticated content" message.
  6. Visit localhost:3000/authpage again and you will see the authenticated content and the log out link.

Contents of src/pages/authpage.tsx

import {client} from 'client'

export default function MyPage() {
    const { isLoading: isAuthLoading, isAuthenticated } = client.auth.useAuth({
        shouldRedirect: true,
    })

    const { isLoggedOut, logout } = client.auth.useLogout()


    /**
     * Not authenticated
     */
    if ( !isAuthenticated || isLoggedOut ) {
        return <div>Not authenticated</div>
    }

    return (
        <div>
            <p>My auth content</p>
            <a href="#" onClick={() => logout()}>Log out</a>
        </div>
    )
}

Link to code example: https://github.com/cfg-fadica/faustjs-logout-demo

The current behavior

User remains logged in after using the logout() function exported from client.auth.useLogout(). The frontend app cookie WordPress authentication persists.

The expected behavior

When the logout() function is fired, frontend app cookie would be destroyed and user would be logged out, unable to access the authenticated content without logging in again.

EDIT:
When the logout() function is fired, WordPress cookies would be destroyed and user would be logged out, unable to access the authenticated content without logging in again.

@theodesp theodesp added type: bug Issue that causes incorrect or unexpected behavior needs: reproduction This issue needs to be reproduced independently and removed type: bug Issue that causes incorrect or unexpected behavior labels Nov 16, 2022
@theodesp
Copy link
Member

theodesp commented Nov 16, 2022

Hey @cfg-fadica. Thank you for mentioning this one. I think this is the expected result when using this hook and providing the shouldRedirect: true,.

It's true that clicking logout will clear the auth and the cookie for that instance. However once you reload the page it will try to authenticate again using redirect.

You see according to the docs here when you use this hook into your page and provide the shouldRedirect: true it will automatically try to login with your WordPress instance. Since you've been already logged in with WordPress, then this process will happen again once you reload the page.

If you logout from WordPress then the whole process is reversed and you will have to login again.

If you wish to disable redirects on load you should use the shouldRedirect: false so in that case when you re-load the page then you are still not authenticated and the hook won't try to login again be default.

Screenshot 2022-11-16 at 10 59 52

Now if you wish to control this better you should be using a login button instead of having useAuth here and try to stop the redirect from happening.

I hope this paints a better picture.

@theodesp theodesp added type: question An issue that involve inquiries, clarifications, or requests for guidance package: core (legacy) Related to the legacy core package and removed needs: reproduction This issue needs to be reproduced independently labels Nov 16, 2022
@ghost
Copy link
Author

ghost commented Nov 16, 2022

Hi @theodesp,
Thank you for your response. I am confused by your response, but what I think I have determined is that I think the bug I am reporting is logout() should destroy the WordPress cookies in a redirect authentication model.

Do you mean that I have to implement my own separate log out from WordPress in order for this to work? I thought that logout() would do that too. I am really confused as to what the logout() function actually accomplishes in this case.

I've used logout() successfully with local authentication, but not with redirect authentication. I have a case where I need to switch my auth type for my application. To me it would seem that logout() would destroy the WordPress cookie(s), thus redirecting a user to the log in page correctly.

So, in conclusion, I am reporting that the actual bug is that WordPress cookies are not destroyed.

@jonjakoblich
Copy link

Hi there. I had to switch accounts. I am the ghost above, formerly cfg-fadica.

@theodesp
Copy link
Member

@jonjakoblich When you call logout, the framework invalidates the refresh token here and clears the cookie.

You can verify this by loading the page using shouldRedirect: true, so that the cookie is populated:
Screenshot 2022-11-17 at 10 46 53

Then go to your code and change shouldRedirect: false, and then try to logout. You will see that after that there will be no cookie in the page:
Screenshot 2022-11-17 at 10 46 40

If you reload the page with shouldRedirect: false then you still be not authenticated. The tricky part to understand is that when you use this code in the page:

const { isLoading: isAuthLoading, isAuthenticated } = client.auth.useAuth({
        shouldRedirect: true,
    })

It will try to authenticate, get the access token and populate the auth cookie every time. When you click logout it will temporarily clear the cookie for a moment but then the whole process will start again as the shouldRedirect: true,will be still there.

Check this recording:

Screen.Recording.2022-11-17.at.10.52.42.mov

@jonjakoblich
Copy link

Hi @theodesp. Thank you for this. I replicated the same steps you took using shouldRedirect: true and captured it on video.

Faust.Bug.mp4

Faust does not terminate the WordPress session, and to me it seems that it should have been terminated on logout(). Therefore, when the page redirects it sees that the user is still authenticated in WordPress and then generates a valid refresh token giving the appearance that the user never logged out from the frontend app. This seems like a bug.

What is the effective way to log out a user using shouldRedirect: true?

@theodesp
Copy link
Member

Hey @jonjakoblich. Thats is true. We don't destroy the session there. Maybe this is a good feature request that we may investigate. I will create a ticket in our backlog.

As for using the shouldRedirect: true I think the best way is to have a button for specifically login once unauthenticated and use a combination of useState and useEffect. Here how it should look like roughly:

import * as React from 'react';
import {client} from 'client'

export default function MyPage() {
    // start with no redirect
    const [shouldRedirect, setShouldRedirect] = React.useState(false);
    const { isLoading: isAuthLoading, isAuthenticated } = client.auth.useAuth({
        shouldRedirect
    });

    const login = () => {
        // redirect to login once user clicks button.
        setShouldRedirect(true);
    };

    React.useEffect(() => {
        // Once authenticated then disable the automatic redirect.
        if (isAuthenticated && shouldRedirect) {
            setShouldRedirect(false);
        }
    }, [shouldRedirect, isAuthenticated])

    const { isLoggedOut, logout } = client.auth.useLogout()

    /**
     * Not authenticated
     */
    if ( !isAuthenticated || isLoggedOut ) {
        return <div>Not authenticated. <a href="#" onClick={() => login()}>Login</a></div>
    }

    return (
        <div>
            <p>My auth content</p>
            
            <a href="#" onClick={() => logout()}>Log out</a>
        </div>
    )
}

I've added a recording. for this functionality:

Screen.Recording.2022-11-17.at.16.28.34.mov

@jonjakoblich
Copy link

jonjakoblich commented Nov 18, 2022

Hi @theodesp. Thank you for that work around. As your video demonstrates, and as I found in testing it, the user is never really logged out because the WordPress cookie still persists, thus on redirect they are not taken to a log in page.

The specific problem I am trying to solve is to have a user both logged in to the front end and the back end without having to log in separately. I was hoping redirect authentication would address this, but its tradeoff is that the user is never logged out from the backend. I prefer using the local authentication because I can create my own login page, but logging in a user to the backend at the same time is proving challenging.

My work around for now is to adapt your solution above to the following.

import {client} from 'client'
import { useRouter } from 'next/router'
import { useEffect, useState } from 'react'

export default function MyPage() {
    const router = useRouter()
    const [ shouldRedirect, setShouldRedirect ] = useState(false)
    const { isLoading: isAuthLoading, isAuthenticated } = client.auth.useAuth({
        shouldRedirect,
    })

    useEffect(() => {
        if( isAuthenticated && shouldRedirect ) {
            setShouldRedirect(false);
        }
        if( !isAuthenticated ) {
            setShouldRedirect(true);
        }
    }, [shouldRedirect, isAuthenticated])

    const { isLoggedOut, logout } = client.auth.useLogout()

    const customLogout = () => {
        logout().then((res) => {
            router.push(process.env.NEXT_PUBLIC_WORDPRESS_URL + '/wp-login.php?action=logout')
       })
    }

    /**
     * Not authenticated
     */
    if ( !isAuthenticated || isLoggedOut ) {
        return <div>Not authenticated</div>
    }

    return (
        <div>
            <p>My auth content</p>
            <a href="#" onClick={() => customLogout()}>Log out</a>
        </div>
    )
}

And on the back end, in a custom plugin I added the following code to eliminate the nonce check (and prompt when it is missing) on log out.

/**
 * Allow logout without _wpnonce confirmation
 */
add_action('check_admin_referer', function ($action, $result) {
    if ( $action == "log-out" && !isset( $_GET['_wpnonce'] ) ) {
        $redirect_to = isset( $_REQUEST['redirect_to'] ) ? $_REQUEST['redirect_to'] : $_SERVER['HTTP_REFERRER'];
        $location = str_replace( '&amp;', '&', wp_logout_url( $redirect_to ) );
        header( "Location: $location" );
        die;
    }
}, 10, 2);

I appreciate that you added my request to the backlog and I look forward to a solution that addresses this issue without a workaround.

@theodesp
Copy link
Member

Sure I've added a ticket for this and will follow up soon.

@theodesp theodesp added type: feature New functionality being added package: faustwp Related to the companion WordPress plugin has jira package: @faustwp/core Related to the Faust Core package and removed type: question An issue that involve inquiries, clarifications, or requests for guidance labels Nov 21, 2022
@JEverhart383
Copy link

@Fran-A-Dev and I experienced this behavior as well when using redirect-based auth to answer a question in Discord as an FYI

@phil-sola
Copy link

Just chiming in here as well to say it would be great to see this feature added to the framework.

I’ve just had this issue today when messaging on Discord. It’s pretty confusing, especially for customers on the front end who won’t have access to the backend to logout in both places.

Would love to see this behaviour added in without workarounds 😀

@akalex-x
Copy link

Would love to see this implemented as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core (legacy) Related to the legacy core package package: @faustwp/core Related to the Faust Core package package: faustwp Related to the companion WordPress plugin type: feature New functionality being added
Projects
None yet
Development

No branches or pull requests

6 participants