vikunja
/
frontend
Archived
6
39
Fork 49
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

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
Conversation 2 Commits 3 Files Changed 3 +15 -13
3 changed files with 15 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
src/components/misc/ready.vue
View File

@ -52,9 +52,15 @@ import NoAuthWrapper from '@/components/misc/no-auth-wrapper.vue'
import {ERROR_NO_API_URL} from '@/helpers/checkAndSetApiUrl'
import {useOnline} from '@/composables/useOnline'
import {useRouter, useRoute} from 'vue-router'
import {getAuthForRoute} from '@/router'
const router = useRouter()
const route = useRoute()
const store = useStore()
const ready = computed(() => store.state.vikunjaReady)
const ready = ref(false)
const online = useOnline()
const error = ref('')
@ -63,7 +69,12 @@ const showLoading = computed(() => !ready.value && error.value === '')
async function load() {
try {
await store.dispatch('loadApp')
} catch(e: any) {
const redirectTo = getAuthForRoute(route)
if (typeof redirectTo !== 'undefined') {
await router.push(redirectTo)
dpschen marked this conversation as resolved
konrad commented 2022-01-08 14:49:28 +00:00
Review
Copy Link

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.
dpschen commented 2022-01-31 22:00:29 +00:00
Review
Copy Link

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.
konrad commented 2022-02-01 20:27:00 +00:00
Review
Copy Link

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.
😆 1
dpschen commented 2022-02-01 21:27:46 +00:00
Review
Copy Link

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
}
ready.value = true
} catch (e: any) {
error.value = e
}
}

6
src/router/index.ts
View File

@ -575,11 +575,7 @@ const router = createRouter({
],
})
router.beforeEach((to) => {
dpschen commented 2022-01-31 22:02:11 +00:00
Outdated
Review
Copy Link

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
konrad commented 2022-02-01 20:23:47 +00:00
Outdated
Review
Copy Link

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.
return checkAuth(to)
})
function checkAuth(route: RouteLocation) {
export function getAuthForRoute(route: RouteLocation) {
konrad marked this conversation as resolved Outdated
dpschen commented 2022-01-31 22:04:14 +00:00
Outdated
Review
Copy Link

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
👍 1
konrad commented 2022-02-01 20:24:05 +00:00
Outdated
Review
Copy Link

Yeah that sounds like a good idea.

Yeah that sounds like a good idea.
konrad commented 2022-02-01 20:25:54 +00:00
Outdated
Review
Copy Link

Renamed.

Renamed.
const authUser = store.getters['auth/authUser']
const authLinkShare = store.getters['auth/authLinkShare']

7
src/store/index.js
View File

@ -43,7 +43,6 @@ export const store = createStore({
menuActive: true,
keyboardShortcutsActive: false,
quickActionsActive: false,
vikunjaReady: false,
},
mutations: {
[LOADING](state, loading) {
@ -79,9 +78,6 @@ export const store = createStore({
[BACKGROUND](state, background) {
state.background = background
},
vikunjaReady(state, ready) {
state.vikunjaReady = ready
},
},
actions: {
async [CURRENT_LIST]({state, commit}, currentList) {
@ -136,10 +132,9 @@ export const store = createStore({
commit(CURRENT_LIST, currentList)
},
async loadApp({commit, dispatch}) {
async loadApp({dispatch}) {
await checkAndSetApiUrl(window.API_URL)
await dispatch('auth/checkAuth')
commit('vikunjaReady', true)
},
},
})