userBean not defined if logged-in user is not found

[michaelborn]

AuthenticationService's getUser() method should be able to handle a null response from the user service.

https://github.com/coldbox-modules/cbauth/blob/main/models/AuthenticationService.cfc#L152-L163 - it seems getUser() expects getUserService().retrieveUserById(...) to throw an error if the user cannot be found.

Instead, I'd prefer to handle null's as a missing user, and logout or clear the session val.

The context of this is that a logged-in user was deleted. (don't ask!)

I would be happy to PR something like this toAuthenticationService.cfc's getUser() method:

if ( isNull( userBean ) ){
    variables.sessionStorage.delete( variables.USER_ID_KEY );
    throw( "User not found" );
}

[elpete]

I think I was dealing with the same scenario:

1. The user is logged in. (sessionStorage has a key set.)
2. The user cannot be retrieved from the database (maybe because it was deleted in tests or something).

Are you suggesting that the catch path also handle null values? I think that's a smart move. The real question is if this should throw. If your retrieveUserById threw an exception we would rethrow. Should null throw? It's definitely an unexpected behavior. But then I'd almost want a consistent exception between the two paths and wrapping exceptions are almost always more trouble than they are worth.

[michaelborn]

Are you suggesting that the catch path also handle null values?

Or handle it immediately after the catch{}. (Which is duplication, yes.)

The real question is if this should throw.

Agreed.

If your retrieveUserById threw an exception we would rethrow.

I'm not sure that is expected behavior. While the docs do not explicitly say so, I assumed all UserService methods would return null and we would have the choice at that point to throw or logout or whatever. If retrieveUserById() needs to throw an exception, then that needs to be communicated in the IUserService.cfc docs.

I tend to think a throw is the best course of action here. There is no way to recover from this state, and furthermore most code calling getUser() will assume the user is logged in and available. We either need to throw in getUser() or warn the developer they need to throw from retrieveUserById() and other UserService methods.