GeriLife / wellbeing-client

Client-only code for Wellbeing project.
Apache License 2.0
0 stars 0 forks source link

Adding login, logout and reset password screens #4

Closed shailee-m closed 3 years ago

shailee-m commented 4 years ago

Added app icons also.

shailee-m commented 4 years ago

There is a potential bug here if people access GeriLife from multiple devices. Users can log in on two devices, but can only logout from one device. When attempting to do anything on the second device after logging out from the first, there is an error message that displays:

Screenshot from 2020-09-29 21-59-30

The user is not able to logout from the second device, as their session has already been cleared on the server.

Sure. Will look into it.

shailee-m commented 4 years ago

There is a potential bug here if people access GeriLife from multiple devices. Users can log in on two devices, but can only logout from one device. When attempting to do anything on the second device after logging out from the first, there is an error message that displays:

Screenshot from 2020-09-29 21-59-30

The user is not able to logout from the second device, as their session has already been cleared on the server.

It was an error in the API method. Fixed in this PR. https://github.com/GeriLife/wellbeing/pull/555

brylie commented 3 years ago

Cool. I merged the PR. Is there a way to make that error fail more gracefully? E.g. in case we add a feature to "log out all devices" that would invalidate all access tokens for a specific user.

shailee-m commented 3 years ago

Cool. I merged the PR. Is there a way to make that error fail more gracefully? E.g. in case we add a feature to "log out all devices" that would invalidate all access tokens for a specific user.

Yes, we can add a login check for each API call by introducing a middleware in the server. Better to do it on the server. I will raise a PR for the same.

brylie commented 3 years ago

The client somehow needs to recover from the situation where its authentication token may have been invalidated by the server. E.g. my client in Firefox is not able to reach the logout screen currently, because the authentication token was cleared out by another client.

Error: TypeError: Cannot read property &#39;services&#39; of undefined [500]<br> &nbsp; &nbsp;at Object.userLogout (server/methods/users.js:71:11)<br> &nbsp; &nbsp;at packages/simple_rest.js:383:33<br> &nbsp; &nbsp;at packages/simple_json-routes.js:98:9

The Activities page doesn't work, as would be expected, but the user isn't shown any error message or the login page.

shailee-m commented 3 years ago

The client somehow needs to recover from the situation where its authentication token may have been invalidated by the server. E.g. my client in Firefox is not able to reach the logout screen currently, because the authentication token was cleared out by another client.

Error: TypeError: Cannot read property &#39;services&#39; of undefined [500]<br> &nbsp; &nbsp;at Object.userLogout (server/methods/users.js:71:11)<br> &nbsp; &nbsp;at packages/simple_rest.js:383:33<br> &nbsp; &nbsp;at packages/simple_json-routes.js:98:9

The Activities page doesn't work, as would be expected, but the user isn't shown any error message or the login page.

Yes, I will add a server redirect to the login page. There isn't much to do on the frontend as it doesn't know if the cookie has expired or been invalidated. I will add to this PR if there are any updates to the frontend.

We can also set Http-only cookie in the response header from backend itself so we needn't do and session management on the frontend. But I need to check if it requires not workarounds with the simple-rest module.

shailee-m commented 3 years ago

@brylie I wanted to add a JSON route middleware that validates token for each API call. But I am having trouble using the middleware. I have also opened an issue on their page: https://github.com/stubailo/meteor-rest/issues/159.

For a temporary solution to this, I have created another API in the backend which validates each token. The frontend will keep polling every 10 seconds to validate the auth token.

brylie commented 3 years ago

The temporary solution sounds good. I merged the PR.

I don't expect the issue to be resolved with JSON routes, as the project seems to be dormant. Note, we can consider forking the project, if it would be beneficial to fix the middleware issue.

brylie commented 3 years ago

Will there be changes to this branch, so the client can know when it has been logged out on the server?

shailee-m commented 3 years ago

Will there be changes to this branch, so the client can know when it has been logged out on the server?

I have pushed the polling logic to this branch. (https://github.com/GeriLife/wellbeing-client/pull/4/commits/22558debf637372c1a8478731b499f0e32dbaac7)

Yes I will be good to fork the repository as it is a small change in the code

brylie commented 3 years ago

Rather than pulling, can we use a "middleware" in our client-side HTTP logic? E.g. wrap every authenticated API call to check that the authentication token is valid before sending the authenticated request?

brylie commented 3 years ago

Perhaps something similar to axios-auth-refresh?

brylie commented 3 years ago

Another example would be to use Axios interceptors to validate the authentication token on relevant routes.

brylie commented 3 years ago

Further, it looks like we can add interceptors to custom Axios instance(s). So, we can create an Axios instance for authenticated requests with an interceptor to validate the auth token while having an unauthenticatedRequests Axios instance where we don't want the interceptor to run before requests.

const authenticatedAxios = axios.create();

function validateUserAuthenticationToken() {...};

authenticatedAxios.interceptors.request.use(validateUserAuthenticationToken);
shailee-m commented 3 years ago

Further, it looks like we can add interceptors to custom Axios instance(s). So, we can create an Axios instance for authenticated requests with an interceptor to validate the auth token while having an unauthenticatedRequests Axios instance where we don't want the interceptor to run before requests.

const authenticatedAxios = axios.create();

function validateUserAuthenticationToken() {...};

authenticatedAxios.interceptors.request.use(validateUserAuthenticationToken);

Yes, that's the plan. However, currently, the token is not validated by simple-rest API. And it allows the method to execute. In this method, the UserId is found null and so the method throws an error. So all errors are 500. and so it is a bit difficult to intercept the actual error. If we can get the middleware to work correctly then the interceptors in axios will be useful

shailee-m commented 3 years ago

validateUserAuthToken

You mean before each API call we fire another API to first check the token validity?

brylie commented 3 years ago

Yes. How much overhead would that add? The latency should be tolerable.

brylie commented 3 years ago

Alternatively, the validateAuthToken check could be run in the router?

brylie commented 3 years ago

In effect, all routes except the login screen should have a valid auth token. So, if it is more efficient to check the auth token in the router it may make sense, it would just seem safer to validate the auth token in the request/response cycle (hence interceptor since middleware doesn't seem to work on the server).

shailee-m commented 3 years ago

In effect, all routes except the login screen should have a valid auth token. So, if it is more efficient to check the auth token in the router it may make sense, it would just seem safer to validate the auth token in the request/response cycle (hence interceptor since middleware doesn't seem to work on the server).

It should be in both places. the interceptor and also the router (beforeEach hook). As on all route change, an API call may not be required.

shailee-m commented 3 years ago

Yes. How much overhead would that add? The latency should be tolerable.

Not much. I will push changes today itself

shailee-m commented 3 years ago

Yes. How much overhead would that add? The latency should be tolerable.

Done. Also generated icons for webapp

shailee-m commented 3 years ago

Yes. How much overhead would that add? The latency should be tolerable.

Done in this commit https://github.com/GeriLife/wellbeing-client/pull/4/commits/39d63cb851458c1020dc6d0af7651349ec616a5e.

Also generated icons for webapp