fix: error propagation totp issue display toast #401
Conversation
packages/auth/src/RocketChatAuth.ts
Outdated
| user, | ||
| password, | ||
| code, | ||
| try { |
There was a problem hiding this comment.
Hey, we could catch the error in the EmbeddedChat itself. Was there any specific reason to use try/catch here?
There was a problem hiding this comment.
@abhinavkrin, I added a try-catch block in RocketChatAuth.ts because frontend error handling relies on specific error properties such as 'totp-required', 'Unauthorized', and 'totp-invalid', which are parsed from the server response. In Api.ts, within the packages/auth folder, the ApiError class wraps the server response with a message and endpoint when the status is not OK from the Rocket.Chat server response. Before parsing it into JSON, I check if it's an instance of the ApiError class because then only i can access the response property.
Handling it in EmbeddedChatApi.ts is possible, but it requires exporting the ApiError class from Api.ts in packages/auth and importing it into EmbeddedChatApi.ts in packages/api to check whether this is an instance of ApiError . The approach I took was more straightforward because we already have access to the ApiError class. Additionally, when it is not an instance of ApiError, I rethrow the error, and those cases will be handled as before.
Another reason for this approach is its similarity to the existing pattern in the getCurrentUser function in RocketChatAuth.ts, making the structure consistent rather than exporting and importing a class.
These are the reasons why I did it this way. Thank you!
There was a problem hiding this comment.
Hey @Spiral-Memory I understand, but the package auth is built keeping in mind that it could be used by other projects also. So we would want to have a uniform behavior.
There are other ways you can export the error reason.
You can pass more information though the cause.
or
Add a code property to error object
or
Make use of the response property of ApiError
In short, the error instance should be thrown from the function instead of a simple JSON object.
|
@abhinavkrin , I have explained the reason why it is done like this. Kindly review this and let me know. It addresses the 2FA OTP entry issue, displays a toast error for login failures, and indicates OTP invalid status—all of which are related to server responses. The frontend part has been in place for a while, but it wasn't working correctly due to server responses. Additionally, this PR resolves the issue of the toast message color; the opacity was on the overlay color, but it was expected to be a white background. Also, the z-index has been fixed. |
packages/auth/src/RocketChatAuth.ts
Outdated
| user, | ||
| password, | ||
| code, | ||
| try { |
There was a problem hiding this comment.
Hey @Spiral-Memory I understand, but the package auth is built keeping in mind that it could be used by other projects also. So we would want to have a uniform behavior.
There are other ways you can export the error reason.
You can pass more information though the cause.
or
Add a code property to error object
or
Make use of the response property of ApiError
In short, the error instance should be thrown from the function instead of a simple JSON object.
|
Got it @abhinavkrin I will do that and let you know. |
Spiral-Memory
force-pushed
the
fix/error-propagation_totp-issue_display-toast
branch
from
479ff17
to
d1e7763
Compare
exported ApiError from auth fixed background color and z-Index of toastBar
Spiral-Memory
force-pushed
the
fix/error-propagation_totp-issue_display-toast
branch
from
d1e7763
to
6177621
Compare
|
Hey @abhinavkrin, I am no longer handling it in I have tested it and everything is working fine as expected. |
Spiral-Memory
changed the title
Fixed the authentication flow for situations where 2FA is enabled, addressed the propagation of error messages so that error message will be displayed when password is incorrect, and also resolved the z-index issue related to the displayed error message.
Acceptance Criteria fulfillment
[x] Resolved the issue with the opening of the TOTP modal when 2FA is enabled, prompting users to enter OTP for authentication.
[x] Addressed the proper propagation of required errors from auth to the API and frontend to ensure successful implementation and incorrect credentials error will be displayed.
[x] Additonally, fixed the z-index problem with the toast message, ensuring it now appears above the overlay (Modal Backdrop) instead of below it.
Fixes #400
Fixes #407
Video/Screenshots
2024-01-16.00-41-21.mp4