Open GabrielGil opened 3 years ago
We really need this. In order not to break current API, I suggest that checkAuth
will only throw an error if a specific param was supplied.
In V12, we added an errorMessage
to the checkAuth
return value. This won't be a boolean
alone anymore, but an object with more information.
I think throwing the error through an observable is a nice idea when logging in. The errors mentioned above can be thrown when logging in or when checking the auth (so coming back from the sts), is that correct?
Thanks
Just checked: Can you provide a sample with a config from our samples and a reproducible error? What I do see is an error at the server, but we need it when coming back. If you provide a sample with an error, we can have a look.
Yes @FabianGosebrink. The errors can come when logging-in or when checking the auth (hence, also when refreshing with the iframe).
The errors from the STS are included in the callback URL, so the library should check those and add them to the error object thrown. I have to check whether I can provide you with some reproducible errors. Let me get back to you at a later stage, please. :)
We would have to map the errors in the url to a consistent error message. We already have a "new" error property in V12. Maybe this is doing it well, but I can test with your example then. Thanks!
In the case of IAT rejected as Gabriel mentioned, when using checkAuthIncludingServer() the errorMessage provided in the LoginResponse is undefined.
[WARN] 0-Client - authCallback Validation, iat rejected id_token was issued too far away from the current time [DEBUG] 0-Client - authCallback token(s) invalid [WARN] 0-Client - authorizedCallback, token(s) validation failed, resetting. Hash: [ERROR] 0-Client - Error: Error: null --at this point LoginResponse errorMessage = undefined.
I am looking forward to the full error handling feature to support all the different error scenarios so we can log to developers and notify users what is actually happening.
Is this issue still relevant?
I think it is. There's only an error property in the response, not really that it handles the different scenarios.
We are handling the different scenarios or events with the PublicEventsService
. This, however, is not only an error service. So we could use this Service and on error case, we send one of those events mentioned above. IN addition to that, we can have a lastError$
observable. Would that be okay or shouldn't we send error in the PublicEventsService
, only "positive" events?
The main problem I see with this is not actually the way errors are returned, but the errors themselves, as they are not typed at all. Most of the errors are part of the spec, and can be typed. Maybe a union, for those errors that cannot be mapped, so default to a generic object. The known errors should be objects, with errors codes along message descriptions. This is to allow the client app to identify the error by code, and not necessarily use the string (or show it in the console). Showing it to the client will not work for i18n apps.
In my experience, the library just ignores the error message returned from the server. I have a failed login, redirecting to something like this:
http://localhost:4200/?error=invalid_request&error_description=MSIS9719%3a+The+code_challenge_method+is+not+supported.&state=...
Instead of containing the error message from the url. the checkAuth produces the error message Error: no code in url
. While this is technically correct it is not particularly relevant since that will always be the case when there is an error like this.
Is your feature request related to a problem? Please describe.
I have successfully implemented this library in my project, however like in any other real-live app, errors do happen. Sometimes these errors are beyond the scope of our implementation, but still, they frustrate the experience of the users who do not know what to do.
Currently, when an error happens logging in the user is just redirected to the
unauthorizedRoute
with a generic "Unauthotized" message and error screen. There is no way to inform the user of what happened, since both this library, or the Authentication server may return an error.I cannot find any mention to error handling in the docs.
Describe the solution you'd like Ideally, this library offers a mechanism to access the error returned, wither in the subscription observer of the
checkAuth
method, or as an event, or any other way the responsible team consider.The most common error that happens in our app is "iat validation", but we cannot identify this error and inform the users that their time is not correct, and they have to fix this. We noticed more people than expected have their system time set ahead, in order to be always on time for their meetings and appointments.
However, other errors are found such a code which was issued with a different state etc.
Moreover, OIDC spec defines a set of errors that the server may return, which ideally can be accessed through the library (
error
,error_description
,error_uri
,state
).Ref: https://openid.net/specs/openid-connect-core-1_0.html#AuthError
Describe alternatives you've considered
I thought about these possible solutions:
Subscriber observer error.
This error could be sent to the "Unauthorized" route using
OidcService.lastError$
for instance, where the last error would be available so the route can either handle it, or display it to the user in a UX-friendly way.