feat: persist link share auth rule in url hash #3336

Merged
konrad merged 11 commits from feature/persist-link-share-hash-in-url into main 2023-06-19 14:26:35 +00:00
5 changed files with 77 additions and 22 deletions

View File

@ -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)
}

View File

@ -5,22 +5,31 @@ export function useRedirectToLastVisited() {
const router = useRouter()
function redirectIfSaved() {
function getLastVisitedRoute() {
dpschen marked this conversation as resolved Outdated

Can we save the redirect url in the url instead of local storage?
I think one usually does this with oath.

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()

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.

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

Created a new issue: https://kolaente.dev/vikunja/frontend/issues/3339
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

Reverse order, so that function ends in 'happy path' (?):

if (last === null) {
	return null
}
Reverse order, so that function ends in 'happy path' (?): ``` if (last === null) { return null } ```

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

Same here.

Same here.

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.

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'

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'

Changed it again, please check

Changed it again, please check
return router.push({name: 'home'})
}
return router.push(lastRoute)
}
return {
redirectIfSaved,
getLastVisitedRoute,
}
}

View File

@ -0,0 +1 @@
export const LINK_SHARE_HASH_PREFIX = '#share-auth-token='

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.

## 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.

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=.

👍

> 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=. 👍

View File

@ -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

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?

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?

Since this is something new, how about only using #share in the url to make it shorter?

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

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)?

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

I think this should be possible now with vue-router 4.2.0. See 909c45be19

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

View File

@ -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

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.

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.

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

Return the push here and below

Return the push here and below

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