fix: authprovider error message when a user is not found in the auth.… #4567
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
DeepDiver1975
force-pushed
the
fix/authprovider-error-message
branch
from
ee0e6d5
to
2764675
Compare
There was a problem hiding this comment.
Please note that not all authproviders treat the ClientID parameter as a username. For auth-machine e.g. the ClientID can be either a username or a <attribute>:<value> tuple, auth-service expectes a "userid" and auth-oidc doesn't look into it at all.
I guess that is why the it was called client_id and not username in the first place.
|
When a users wants to login and this fails and the log holds
|
| @@ -149,9 +149,9 @@ func (s *service) Authenticate(ctx context.Context, req *provider.AuthenticateRe | |||
| Status: status.NewPermissionDenied(ctx, v, "wrong password"), | |||
| }, nil | |||
| case errtypes.NotFound: | |||
| log.Debug().Str("client_id", username).Msg("unknown client id") | |||
| log.Debug().Str("username", username).Msg("unknown user") | |||
There was a problem hiding this comment.
How about:
| log.Debug().Str("username", username).Msg("unknown user") | |
| log.Debug().Str("client_id", username).Err(err).Msg("unknown client id or user") |
?
There was a problem hiding this comment.
What I had in mind after reading your initial comment. 👍
There was a problem hiding this comment.
(There is a double white space in the error message)
There was a problem hiding this comment.
When a users wants to login and this fails and the log holds unknown client id the last thing you assume is that the user is unknown. This is the intent of this change.
Yeah understood. Could we just add some more specific logging to the individual auth-providers? Also they might already return some specific error message. (See suggestion)
unknown client id or user might be better? And leaving the parameter unchanged in the logger ...
Would the suggestion help you?
| return &provider.AuthenticateResponse{ | ||
| Status: status.NewNotFound(ctx, "unknown client id"), | ||
| Status: status.NewNotFound(ctx, "unknown user"), |
There was a problem hiding this comment.
We could just bubble up err.Error() here instead for 'unknown user".
DeepDiver1975
force-pushed
the
fix/authprovider-error-message
branch
from
2764675
to
1309f01
Compare
DeepDiver1975
force-pushed
the
fix/authprovider-error-message
branch
from
1309f01
to
3030aa4
Compare
…Manager