Improve error when token storage fails during sign-in

[mbuhot]

Somewhat similar to #370, we recently experienced a situation where the database was stuck in a read-only mode after a failover.

Sign-in was failing with an unhelpful message:

** (MatchError) no match of right hand side value: :error

lib/ash_authentication/strategies/password/sign_in_preparation.ex:132 AshAuthentication.Strategy.Password.SignInPreparation.generate_token/3

The SignInPreparation.generate_token fails with MatchError if Jwt.token_for_user returns :error due to being unable to store the token.

Ideally these functions that we expect to rarely fail (like generating a token) would use ! variants of any actions they call rather than failing to match on :error returns, so that a more detailed exception is recorded.

The reason is lost in the else clause of the with expression at: https://github.com/team-alembic/ash_authentication/blob/main/lib/ash_authentication/jwt.ex#L116

[jimsynz]

Looks like @simpers slipped this in with #749. It now emits an error into the log.

[simpers]

Looks like @simpers slipped this in with #749. It now emits an error into the log.

Slipped in the log, or what do you mean? 😅

I didn't change the signature of the return so as to not break things, but trying to log the reason if possible would at least give us something more, but it still might not help if the reason there is as vague.

Ideally these functions that we expect to rarely fail (like generating a token) would use ! variants of any actions they call rather than failing to match on :error returns, so that a more detailed exception is recorded.

The reason is lost in the else clause of the with expression at: https://github.com/team-alembic/ash_authentication/blob/main/lib/ash_authentication/jwt.ex#L116

I agree with this, and trying to get as much info about the error as possible would be great, if it would be possible to allow it to be returned.

The original error I got was just the same unhelpful message, but in my case adding that log allowed me to find the issue. But if it is still as vague downstream, it might not help in all cases.

[zachdaniel]

Yeah @jimsynz was just pointing out you'd already solved the issue by emitting a log. Well, you fixed the "we can't see what went wrong" bit, but we should change the return values as well.

[simpers]

Yeah @jimsynz was just pointing out you'd already solved the issue by emitting a log. Well, you fixed the "we can't see what went wrong" bit, but we should change the return values as well.

Yeah, I thought as much 😅

I'd be in favour of getting more information here, if possible. If not now, in a release that would accept such a possibly breaking change?