fix: make sure the app is fully ready before trying to redirect to the login page #1337

Manually merged
dpschen merged 3 commits from fix/app-ready into main 2022-02-05 16:49:14 +00:00
Owner

This PR moves the check if the user is authenticated to the ready component to make sure the app is fully loaded before redirecting the user to the login page.

Before this PR, Vikunja would first redirect to login and then, once everything was ready, back to the page the user wanted to visit. Because all of happened while the loading spinner overlay was visible, one would not notice.

However, when getting redirected from migrators, the get parameters of that redirect would be lost. That essentially stopped all migrators from working correctly.

Fixes #1264

This PR moves the check if the user is authenticated to the `ready` component to make sure the app is fully loaded before redirecting the user to the login page. Before this PR, Vikunja would first redirect to login and then, once everything was ready, back to the page the user wanted to visit. Because all of happened while the loading spinner overlay was visible, one would not notice. However, when getting redirected from migrators, the get parameters of that redirect would be lost. That essentially stopped all migrators from working correctly. Fixes #1264
konrad added 1 commit 2022-01-08 14:47:52 +00:00
konrad reviewed 2022-01-08 14:49:28 +00:00
@ -66,1 +72,3 @@
} catch(e: any) {
const redirectTo = checkAuth(route)
if (typeof redirectTo !== 'undefined') {
await router.push(redirectTo)
Author
Owner

I wanted to make sure the app would only be ready after it was redirected. What bothers me here is I had to move the store.commit('vikunjaReady', true) out of the loadApp action. It should be fine since this is the only place where that action is used, but I'm not quite sure about it.

I wanted to make sure the app would only be ready after it was redirected. What bothers me here is I had to move the `store.commit('vikunjaReady', true)` out of the `loadApp` action. It should be fine since this is the only place where that action is used, but I'm not quite sure about it.

Can you give a bit more context what you mean by:

I wanted to make sure the app would only be ready after it was redirected.

Can you give a bit more context what you mean by: > I wanted to make sure the app would only be ready after it was redirected.
Author
Owner

That was meant the other way around - "...make sure the app would only redirect after it was ready"

See my lenghty comment below.

That was meant the other way around - "...make sure the app would only redirect after it was ready" See my lenghty comment below.

Ahh that makes more sense. I really broke my mind on this one :D

Ahh that makes more sense. I really broke my mind on this one :D
dpschen marked this conversation as resolved
Member

Hi konrad!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1337-fixapp-ready--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi konrad! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://1337-fixapp-ready--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
dpschen reviewed 2022-01-31 22:02:11 +00:00
@ -575,11 +575,7 @@ const router = createRouter({
],
})
router.beforeEach((to) => {

This function was meant to check the authentification on each navigation step. If we remove it the user will be able to navigate even if he shouldn't be allowed.

See: https://next.router.vuejs.org/guide/advanced/navigation-guards.html#global-before-guards

This function was meant to check the authentification on each navigation step. If we remove it the user will be able to navigate even if he shouldn't be allowed. See: https://next.router.vuejs.org/guide/advanced/navigation-guards.html#global-before-guards
Author
Owner

The problem with the way this was implemented is it will always redirect to /login while the application is not yet loaded and it does not yet know whether the user is logged in or not. Then, once it knows that, the beforeEach handler would allow the request and redirect to the page the user last visited (but I think the last part happens in App.vue or related).

There are two problems with that:

  1. It creates a new history entry for /login even though the user did not activeley navigates there. This creates problems when I re-open or reload a browser tab with Vikunja in it which has old entries and then want to navigate back to those entries. I have to go look in the browser history to get there.
    Example: User navigates from / to /1 to /2 to /3. The history after this navigation is ['/', '/1', '/2', '/3']. Then thy reload the page with an under-the-hood redirect happening to /login and back to /3. The browser history now is ['/', '/1', '/2', '/3', '/login', '/3'] - when the user then presses the back button and expects to get to /2 they go to /login and get redirected immediately to /3. This is highly confusing.
  2. When redirecting after the login back to the page the user was previously on, it looses all query parameters the url might have had before. This completely breaks external integrations like migrators or password resets.

While the second one would be fixable through other means, the first one would require a lot more work with little additional benefit over the solution I implemented in this PR.

Because I moved the check to the <ready> component instead, the application will only check whether or not to redirect the user if it is possible to know whether the user is logged in or now. This avoids the redirect to /login and back while preventing unauthenticated users to access the app.

The problem with the way this was implemented is it will always redirect to `/login` while the application is not yet loaded and it does not yet know whether the user is logged in or not. Then, once it knows that, the `beforeEach` handler would allow the request and redirect to the page the user last visited (but I think the last part happens in `App.vue` or related). There are two problems with that: 1. It creates a new history entry for `/login` even though the user did not activeley navigates there. This creates problems when I re-open or reload a browser tab with Vikunja in it which has old entries and then want to navigate back to those entries. I have to go look in the browser history to get there. Example: User navigates from `/` to `/1` to `/2` to `/3`. The history after this navigation is `['/', '/1', '/2', '/3']`. Then thy reload the page with an under-the-hood redirect happening to `/login` and back to `/3`. The browser history _now_ is `['/', '/1', '/2', '/3', '/login', '/3']` - when the user then presses the back button and expects to get to `/2` they go to `/login` and get redirected immediately to `/3`. This is highly confusing. 2. When redirecting after the login back to the page the user was previously on, it looses all query parameters the url might have had before. This completely breaks external integrations like migrators or password resets. While the second one would be fixable through other means, the first one would require a lot more work with little additional benefit over the solution I implemented in this PR. Because I moved the check to the `<ready>` component instead, the application will only check whether or not to redirect the user if it is possible to know whether the user is logged in or now. This avoids the redirect to `/login` and back while preventing unauthenticated users to access the app.
dpschen reviewed 2022-01-31 22:04:14 +00:00
@ -580,3 +578,1 @@
})
function checkAuth(route: RouteLocation) {
export function checkAuth(route: RouteLocation) {

Regardless of the above: I confused myself by naming this function the same way as the checkAuth store action.
Maybe this here should be called getAuthForRoute What do you think

Regardless of the above: I confused myself by naming this function the same way as the `checkAuth` store action. Maybe this here should be called `getAuthForRoute` What do you think
Author
Owner

Yeah that sounds like a good idea.

Yeah that sounds like a good idea.
Author
Owner

Renamed.

Renamed.
konrad marked this conversation as resolved
konrad added 1 commit 2022-02-01 20:25:50 +00:00
continuous-integration/drone/pr Build is passing Details
dfa30258aa
chore: rename function
dpschen added 1 commit 2022-02-01 22:10:04 +00:00
continuous-integration/drone/pr Build is passing Details
24a154422d
chore: remove vikunjaReady from store
Author
Owner

Ah so we don't need the vikunjaReady state anymore in store because the ready component is the only place it is used?

Ah so we don't need the vikunjaReady state anymore in store because the ready component is the only place it is used?
dpschen manually merged commit 9a5b5c688d into main 2022-02-05 16:49:14 +00:00
konrad deleted branch fix/app-ready 2022-02-05 16:49:36 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.