feat: persist link share auth rule in url hash #3336
|
@ -56,11 +56,13 @@ import {useOnline} from '@/composables/useOnline'
|
|||
import {getAuthForRoute} from '@/router'
|
||||
|
||||
import {useBaseStore} from '@/stores/base'
|
||||
import {useAuthStore} from '@/stores/auth'
|
||||
|
||||
const router = useRouter()
|
||||
const route = useRoute()
|
||||
|
||||
const baseStore = useBaseStore()
|
||||
const authStore = useAuthStore()
|
||||
|
||||
const ready = computed(() => baseStore.ready)
|
||||
const online = useOnline()
|
||||
|
@ -72,7 +74,7 @@ async function load() {
|
|||
try {
|
||||
await baseStore.loadApp()
|
||||
baseStore.setReady(true)
|
||||
const redirectTo = await getAuthForRoute(route)
|
||||
const redirectTo = await getAuthForRoute(route, authStore)
|
||||
if (typeof redirectTo !== 'undefined') {
|
||||
await router.push(redirectTo)
|
||||
}
|
||||
|
|
|
@ -5,22 +5,31 @@ export function useRedirectToLastVisited() {
|
|||
|
||||
const router = useRouter()
|
||||
|
||||
function redirectIfSaved() {
|
||||
function getLastVisitedRoute() {
|
||||
dpschen marked this conversation as resolved
Outdated
|
||||
const last = getLastVisited()
|
||||
if (last !== null) {
|
||||
router.push({
|
||||
name: last.name,
|
||||
params: last.params,
|
||||
query: last.query,
|
||||
})
|
||||
clearLastVisited()
|
||||
return
|
||||
if (last === null) {
|
||||
dpschen marked this conversation as resolved
Outdated
dpschen
commented
Reverse order, so that function ends in 'happy path' (?):
Reverse order, so that function ends in 'happy path' (?):
```
if (last === null) {
return null
}
```
konrad
commented
Done. Done.
|
||||
return null
|
||||
}
|
||||
|
||||
router.push({name: 'home'})
|
||||
clearLastVisited()
|
||||
return {
|
||||
name: last.name,
|
||||
params: last.params,
|
||||
query: last.query,
|
||||
}
|
||||
}
|
||||
|
||||
function redirectIfSaved() {
|
||||
const lastRoute = getLastVisitedRoute()
|
||||
if (!lastRoute) {
|
||||
dpschen marked this conversation as resolved
Outdated
konrad
commented
I changed it, but I'm not sure if how I changed it is what you have in mind. Please check again. I changed it, but I'm not sure if how I changed it is what you have in mind. Please check again.
dpschen
commented
Obviously all very picky, but just to explain: Happy path is defined by function name => Happy action is successful redirect. Obviously all very picky, but just to explain:
Happy path is defined by function name => Happy action is successful redirect.
Redirection to 'home' should be 'fail early'
konrad
commented
Changed it again, please check Changed it again, please check
|
||||
return router.push({name: 'home'})
|
||||
}
|
||||
|
||||
return router.push(lastRoute)
|
||||
}
|
||||
|
||||
return {
|
||||
redirectIfSaved,
|
||||
getLastVisitedRoute,
|
||||
}
|
||||
}
|
|
@ -0,0 +1 @@
|
|||
export const LINK_SHARE_HASH_PREFIX = '#share-auth-token='
|
||||
dpschen
commented
1) Name is conflicting with pathnameI just realised that this new name now conflicts a bit with the name of the link share route, like
So is there any moment where both 2) Use a more describing namingTLDR: While I prefer short urls - keeping them understandable is even more important. The share hash could now be mistaken as some kind of tracking hash (afaik that doesn't even exist, but not important for this case), so users might remove the hash with the intend to remove unimportant info and render the url unusable.
While we try to keep the users data as private as possible and don't want to do such url tracking, the user doesn't know this. ## 1) Name is conflicting with pathname
I just realised that this new name now conflicts a bit with the name of the link share route, like
> `https://my.vikunja.cloud/`**`share`**`/UrdhKPqumxDXUbYpEGJLSIyNTwAnbBzVlwdDpRbv/auth?view=kanban`
So is there any moment where both `share` parmeters (in the pathname and in the hash) could exist in the url at the same time?
And if so does that scenario make sense? In case it doesn't we should prevent that.
## 2) Use a more describing naming
**TLDR:**
Despite beeing longer, replace with a clearer name like `#share-auth-token=`.
While I prefer short urls - keeping them understandable is even more important. The share hash could now be mistaken as some kind of tracking hash (afaik that doesn't even exist, but not important for this case), so users might remove the hash with the intend to remove unimportant info and render the url unusable.
- I thought of this,
- disregarded the idea,
- thought of it again while imagening myself seeing somethink like `#share=ab12de34ef...` in the url and
- finally realised, that the chance might be higher than my initial judgement.
While we try to keep the users data as private as possible and don't want to do such url tracking, the user doesn't know this.
konrad
commented
I can't think of a scenario where that might happen, other than the user manually changing the url. But even in that case, it will just be ignored.
👍 > So is there any moment where both share parmeters (in the pathname and in the hash) could exist in the url at the same time?
And if so does that scenario make sense? In case it doesn't we should prevent that.
I can't think of a scenario where that might happen, other than the user manually changing the url. But even in that case, it will just be ignored.
> Despite beeing longer, replace with a clearer name like #share-auth-token=.
👍
|
|
@ -6,6 +6,7 @@ import {saveProjectView, getProjectView} from '@/helpers/projectView'
|
|||
import {parseDateOrString} from '@/helpers/time/parseDateOrString'
|
||||
import {getNextWeekDate} from '@/helpers/time/getNextWeekDate'
|
||||
import {setTitle} from '@/helpers/setTitle'
|
||||
import {LINK_SHARE_HASH_PREFIX} from '@/constants/linkShareHash'
|
||||
|
||||
import {useProjectStore} from '@/stores/projects'
|
||||
import {useAuthStore} from '@/stores/auth'
|
||||
|
@ -80,7 +81,7 @@ const router = createRouter({
|
|||
}
|
||||
|
||||
// Scroll to anchor should still work
|
||||
if (to.hash) {
|
||||
if (to.hash && !to.hash.startsWith(LINK_SHARE_HASH_PREFIX)) {
|
||||
dpschen marked this conversation as resolved
Outdated
dpschen
commented
Write in screaming snake case, so that it's clear that this is a const: Write in screaming snake case, so that it's clear that this is a const: `LINK_SHARE_HASH_PREFIX`. Since this is something new, how about only using `#share` in the url to make it shorter?
konrad
commented
I like that, renamed it. > Since this is something new, how about only using #share in the url to make it shorter?
I like that, renamed it.
|
||||
return {el: to.hash}
|
||||
}
|
||||
|
||||
|
@ -442,8 +443,7 @@ const router = createRouter({
|
|||
],
|
||||
})
|
||||
|
||||
export async function getAuthForRoute(route: RouteLocation) {
|
||||
const authStore = useAuthStore()
|
||||
export async function getAuthForRoute(to: RouteLocation, authStore) {
|
||||
if (authStore.authUser || authStore.authLinkShare) {
|
||||
return
|
||||
}
|
||||
|
@ -464,26 +464,52 @@ export async function getAuthForRoute(route: RouteLocation) {
|
|||
'user.register',
|
||||
'link-share.auth',
|
||||
'openid.auth',
|
||||
].includes(route.name as string) &&
|
||||
].includes(to.name as string) &&
|
||||
localStorage.getItem('passwordResetToken') === null &&
|
||||
localStorage.getItem('emailConfirmToken') === null &&
|
||||
!(route.name === 'home' && (typeof route.query.userPasswordReset !== 'undefined' || typeof route.query.userEmailConfirm !== 'undefined'))
|
||||
!(to.name === 'home' && (typeof to.query.userPasswordReset !== 'undefined' || typeof to.query.userEmailConfirm !== 'undefined'))
|
||||
) {
|
||||
saveLastVisited(route.name as string, route.params, route.query)
|
||||
saveLastVisited(to.name as string, to.params, to.query)
|
||||
return {name: 'user.login'}
|
||||
}
|
||||
|
||||
if(localStorage.getItem('passwordResetToken') !== null && route.name !== 'user.password-reset.reset') {
|
||||
if(localStorage.getItem('passwordResetToken') !== null && to.name !== 'user.password-reset.reset') {
|
||||
return {name: 'user.password-reset.reset'}
|
||||
}
|
||||
|
||||
if(localStorage.getItem('emailConfirmToken') !== null && route.name !== 'user.login') {
|
||||
if(localStorage.getItem('emailConfirmToken') !== null && to.name !== 'user.login') {
|
||||
return {name: 'user.login'}
|
||||
}
|
||||
}
|
||||
|
||||
router.beforeEach(async (to) => {
|
||||
return getAuthForRoute(to)
|
||||
router.beforeEach(async (to, from) => {
|
||||
const authStore = useAuthStore()
|
||||
|
||||
if(from.hash && from.hash.startsWith(LINK_SHARE_HASH_PREFIX)) {
|
||||
to.hash = from.hash
|
||||
dpschen
commented
Out of interest: Why did you pass the authStore (makes usage e.g. in ready.vue more complex)? Out of interest: Why did you pass the authStore (makes usage e.g. in ready.vue more complex)?
konrad
commented
Not sure anymore, I think something didn't work when creating the store inline Not sure anymore, I think something didn't work when creating the store inline
dpschen
commented
I think this should be possible now with vue-router 4.2.0. See I think this should be possible now with vue-router 4.2.0. See https://github.com/vuejs/router/commit/909c45be199fbf48e55f3f7e681e93c259e82a14
|
||||
}
|
||||
|
||||
if (to.hash.startsWith(LINK_SHARE_HASH_PREFIX) && !authStore.authLinkShare) {
|
||||
saveLastVisited(to.name as string, to.params, to.query)
|
||||
return {
|
||||
name: 'link-share.auth',
|
||||
params: {
|
||||
share: to.hash.replace(LINK_SHARE_HASH_PREFIX, ''),
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
const newRoute = await getAuthForRoute(to, authStore)
|
||||
if(newRoute) {
|
||||
return {
|
||||
...newRoute,
|
||||
hash: to.hash,
|
||||
}
|
||||
}
|
||||
|
||||
if(!to.fullPath.endsWith(to.hash)) {
|
||||
return to.fullPath + to.hash
|
||||
}
|
||||
})
|
||||
|
||||
export default router
|
|
@ -40,12 +40,15 @@ import {useTitle} from '@vueuse/core'
|
|||
|
||||
import Message from '@/components/misc/message.vue'
|
||||
import {PROJECT_VIEWS, type ProjectView} from '@/types/ProjectView'
|
||||
import {LINK_SHARE_HASH_PREFIX} from '@/constants/linkShareHash'
|
||||
|
||||
import {useBaseStore} from '@/stores/base'
|
||||
import {useAuthStore} from '@/stores/auth'
|
||||
import {useRedirectToLastVisited} from '@/composables/useRedirectToLastVisited'
|
||||
|
||||
const {t} = useI18n({useScope: 'global'})
|
||||
useTitle(t('sharing.authenticating'))
|
||||
const {getLastVisitedRoute} = useRedirectToLastVisited()
|
||||
|
||||
dpschen marked this conversation as resolved
Outdated
dpschen
commented
Not something for this pr, but generally: I have the feeling that the whole auth part of this component should be included in the router. There are advantages to have it in its own file though, like not loading this logic until it's needed. Not something for this pr, but generally: I have the feeling that the whole auth part of this component should be included in the router. There are advantages to have it in its own file though, like not loading this logic until it's needed.
konrad
commented
Yes, the whole authentication part has grown over time and likely needs a refactor. Yes, the whole authentication part has grown over time and likely needs a refactor.
dpschen
commented
I have an branch with some refactoring… We'll get to it I have an branch with some refactoring… We'll get to it
|
||||
function useAuth() {
|
||||
const baseStore = useBaseStore()
|
||||
|
@ -87,7 +90,21 @@ function useAuth() {
|
|||
? route.query.view
|
||||
: 'list'
|
||||
|
||||
router.push({name: `project.${view}`, params: {projectId}})
|
||||
const hash = LINK_SHARE_HASH_PREFIX + route.params.share
|
||||
|
||||
const last = getLastVisitedRoute()
|
||||
if (last) {
|
||||
dpschen marked this conversation as resolved
Outdated
dpschen
commented
Return the push here and below Return the push here and below
konrad
commented
Done Done
|
||||
return router.push({
|
||||
...last,
|
||||
hash,
|
||||
})
|
||||
}
|
||||
|
||||
return router.push({
|
||||
name: `project.${view}`,
|
||||
params: {projectId},
|
||||
hash,
|
||||
})
|
||||
} catch (e: any) {
|
||||
if (e.response?.data?.code === 13001) {
|
||||
authenticateWithPassword.value = true
|
||||
|
|
Can we save the redirect url in the url instead of local storage?
I think one usually does this with oath.
If not: rename to
getLastVisitedRoute()
Here I was just reusing the same mechanism we already use elsewhere. I also though about using the url instead of localStorage for the redirect, but that shouldn't happen in this PR. I've now renamed the function instead.
Created a new issue: #3339