authts / react-oidc-context

Lightweight auth library based on oidc-client-ts for React single page applications (SPA). Support for hooks and higher-order components (HOC).
MIT License
706 stars 66 forks source link

`signinRedirect()` vs `signinSilent()` for automatic login #1034

Open pseidemann opened 11 months ago

pseidemann commented 11 months ago

hello,

as I understand from docs and issue responses, signinRedirect() should be used to automatically login the app, e.g. after revisiting the url. furthermore, I understand that signinSilent() is handled by the library.

however, I wonder why signinSilent() shouldn't be used for automatic login. I think it is better if the app tries to login in the background and if that fails, the app stays in the logged-out state and presents the user with a login button which will call signinRedirect().

this would prevent the app from instantly redirecting to the oauth provider, with no option for the user to "cancel" the login and just go back to the app without logging in. it also removes the redirect "flashes", in case the login goes through automatically.

I wonder if this use-case is currently possible without custom code?

I came up with some implementation myself, wondering if it is correct and if there is maybe an already supported, official, easier way.

bonus: I want to sync the logged-in state/access token with my redux store.

this is what I came up with:

export default function App() {
  const auth = useAuth();
  const dispatch = useDispatch();
  const [loginAttempted, setLoginAttempted] = useState(false);
  const loggedIn = useSelector(selectLoggedIn);

  // always try to silently login once via auth code grant with PKCE or refresh token grant
  useEffect(() => {
    if (
      !loginAttempted &&
      !hasAuthParams() &&
      !auth.isAuthenticated &&
      !auth.activeNavigator &&
      !auth.isLoading &&
      !auth.error  // <--- this part is missing in the docs (?)
    ) {
      setLoginAttempted(true);
      auth.signinSilent();
    }
  }, [auth, loginAttempted]);

  // handle login attempt errors
  useEffect(() => {
    if (!auth.error) {
      return;
    }

    if (
      (auth.error instanceof ErrorResponse) &&
      [  // as per https://openid.net/specs/openid-connect-core-1_0.html#AuthError
        'interaction_required',
        'login_required',
        'account_selection_required',
        'consent_required',
      ].includes(auth.error.error)
    ) {
      // oauth provider needs some form of end-user interaction,
      // due to not being logged in, or similar reasons
      // => not a real error => ignore error so user can proceed to click "login" manually
      return;
    }

    // otherwise: report error to user
    dispatch(setError(auth.error));
  }, [dispatch, auth.error]);

  // sync access token with redux
  useEffect(() => {
    const token = auth.isAuthenticated ? auth.user.access_token : null;
    // if null, loggedIn in the redux state gets set to false as well
    dispatch(setToken(token));
  }, [dispatch, auth.isAuthenticated, auth.user]);

  // do some api calls
  useEffect(() => {
    if (!loggedIn) {
      return;
    }

    // do calls to protected apis with token from redux store
    // ...
  }
}

is this a correct implementation?

pamapa commented 11 months ago

Why not, looks good. Its up to each application developer to define what happens when you login the first time. The library seems to give you that flexibility.

I check in the application start up for a valid access token, if so no manual login needed, else show login screen (the user sees some copyright and agreement stuff and must click sign-in).

pseidemann commented 11 months ago

hi @pamapa, thanks for your feedback!

does that mean that your app doesn't use the refresh token, in case the access token is expired, when revisiting the app, forcing the user to re-login?

I forgot to mention that in my question, but another requirement is that the refresh token should be used, as a "fallback", if possible.

pamapa commented 11 months ago

I am trying to not use the refresh token, in case the user authenticated before, the authz server will set a session cookie, which then automates the normal login like using a refresh token...

Tockra commented 9 months ago

Your solution seems to be better. I don't understand the official documentation here. Why does here exists this part:

  React.useEffect(() => {
        if (!hasAuthParams() &&
            !auth.isAuthenticated && !auth.activeNavigator && !auth.isLoading &&
            !hasTriedSignin
        ) {
            auth.signinRedirect();
            setHasTriedSignin(true);
        }
    }, [auth, hasTriedSignin]);

Is hasTriedSignin= true here anytime? If it is set to true I was redirected before to a different website. So I lose all my react states. Then I get a redirection back.

  1. Why should setHasTriedSignin(true); was executed, which cleary is placed after the auth.signinRedirect();
  2. Why the state should be stored even after I routed to a different website and then back?
pseidemann commented 9 months ago

hi @Tockra, I think the code you showed is wrong, for the reasons you mentioned. would be interesting to get some feedback from @pamapa, if this is really a bug in the documentation.

pamapa commented 9 months ago

@Tockra and @pseidemann I agree with you, this is a bug in the documentation, would be nice if you could update this. BTW: I am think if it would not be better to have that within the library itself? Notice that i am not using automatic login with my applications...

pseidemann commented 9 months ago

it would be nice to have this in the library, especially the auth error handling I did (the effect with the // handle login attempt errors comment)

Tockra commented 8 months ago

Okay after overthinking it, I'm sure that the purpose of @pseidemann codesnippet and the code snippet from the documentation I posted is different:

React.useEffect(() => {
        if (!hasAuthParams() &&
            !auth.isAuthenticated && !auth.activeNavigator && !auth.isLoading &&
            !hasTriedSignin
        ) {
            auth.signinRedirect();
            setHasTriedSignin(true);
        }
    }, [auth, hasTriedSignin]);

The documentation aims to root yourself to the login page if you are not logged in. This happens automatically. I think the hasTriedSignin is maybe not necessary, because it only assures that the login does not happen twice. If there is no internal behavior I don't see, I see no reason why it should happen twice if there is no auth.error. If the login page isn't there then you end in a login loop. So an if(!auth.err) check could an alternate. So (MAYBE) if I understand everything right the documentation could become rid of hasTriedSignin and should anyways add (!auth.err).

But the code in the documentation has a different purpose than @pseidemann code. This loggs you in if you are not logged in before but if you are already logged into your login provider (login page redirects you immediatly back).