feat: persist link share auth rule in url hash #3336
No reviewers
Labels
No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
No Milestone
No project
No Assignees
3 Participants
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/frontend#3336
Loading…
Reference in New Issue
No description provided.
Delete Branch "feature/persist-link-share-hash-in-url"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This allows sharing links to a task directly. We're using hashes instead of query parameters because hash values are usually not logged in access logs.
With this change, when a user uses a link share, the link share hash will be appended to all urls while browsing. When a link share hash is encountered in the current url and the user is not authenticated, they will be redirected to the link share auth page, get authenticated and then get redirected to whatever url they were previously on.
Hi konrad!
Thank you for creating a PR!
I've deployed the changes of this PR on a preview environment under this URL: https://3336-feature-persist-link-share-hash---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!
Wow that was fast!
Thanks! I felt like it wouldn't be too complicated so I decided on a whim to implement it :D
@ -6,3 +6,3 @@
const router = useRouter()
function redirectIfSaved() {
function getRedirectRoute() {
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
@ -80,6 +81,8 @@ const NewNamespaceComponent = () => import('@/views/namespaces/NewNamespace.vue'
const EditTeamComponent = () => import('@/views/teams/EditTeam.vue')
const NewTeamComponent = () => import('@/views/teams/NewTeam.vue')
const linkShareHashPrefix = '#linkshare='
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?I like that, renamed it.
@ -48,3 +49,4 @@
useTitle(t('sharing.authenticating'))
const {getRedirectRoute} = useRedirectToLastVisited()
function useAuth() {
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.
I have an branch with some refactoring… We'll get to it
@ -59,6 +61,7 @@ function useAuth() {
const password = ref('')
const authLinkShare = computed(() => authStore.authLinkShare)
const linkShareHashPrefix = '#linkshare='
Import from router
I've moved it to a separate helper file instead and imported that.
Can you move that to the
constants
folder instead?Done.
@ -91,0 +93,4 @@
const hash = linkShareHashPrefix + route.params.share
const last = getRedirectRoute()
if (last) {
Return the push here and below
Done
@ -530,0 +534,4 @@
to.hash = from.hash
}
if (to.hash.startsWith(linkShareHashPrefix)) {
Reduce nesting:
Done.
@ -8,3 +8,3 @@
function redirectIfSaved() {
function getLastVisitedRoute() {
const last = getLastVisited()
if (last !== null) {
Reverse order, so that function ends in 'happy path' (?):
Done.
@ -18,0 +21,4 @@
function redirectIfSaved() {
const lastRoute = getLastVisitedRoute()
if (lastRoute) {
Same here.
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'
Changed it again, please check
@ -90,3 +92,3 @@
// Scroll to anchor should still work
if (to.hash) {
if (to.hash && !to.hash.startsWith(LINK_SHARE_HASH_PREFIX)) {
Unsure: Does
route.hash
contain#
and=
? Because it's part ofLINK_SHARE_HASH_PREFIX)
.From my tests, it contains everything, including
#
and=
.Okay
=
makes sense. I was not aware of#
though.Something learnt 🙂.
@ -527,3 +529,2 @@
router.beforeEach(async (to) => {
return getAuthForRoute(to)
router.beforeEach(async (to, from) => {
Having read this twice now:
I think the content of this hook should be incorporated in
getAuthForRoute
. This might be tricky right now because of thebaseStore.ready
line.Regardless:
If I am already authenticated as a user and I open this link wouldn't I be redirected to the 'link-share.auth' page?
In this context I also stumbled over these lines in the router:
Note the check for
authLinkShare
. This seems like a duplication, because we check the same info in:I tried it and this seems to be a problem. I don't understand why though, it does not work when I move the checks right at the top of that function.
I think the problem here is how
getAuthForRoute
is called in the ready wrapper as well.I'll try aswell
I tried, but failed, for now :/
Still not really happy how this works though. It's really hard to understand and this pr doesn't make it easier to see through the whole auth process. Still trying to implement something that is not too invasive but makes sense.
Then maybe let's move refactoring that to another PR
@ -530,0 +533,4 @@
to.hash = from.hash
}
if (to.hash.startsWith(linkShareHashPrefix) && getToken() === null) {
I think we should read here
authStore.authUser
instead.I'm now using
authStore.authLinkShare
and that seems to work great. Why would you useauthStore.authUser
?Ahh yes, that makes even more sense!
@ -530,0 +547,4 @@
if(newRoute) {
return {
...newRoute,
hash: to.hash,
This would also pass a hash that doesn't start with
LINK_SHARE_HASH_PREFIX
.But the check in Line 531 will mean
to.hash
is either what was intented previously or the share link hash. So either nothing really changed or the other link share hash gets passed.@ -530,0 +552,4 @@
}
if(!to.fullPath.endsWith(to.hash)) {
return to.fullPath + to.hash
Same here.
@ -0,0 +1 @@
export const LINK_SHARE_HASH_PREFIX = '#share='
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
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.
#share=ab12de34ef...
in the url andWhile 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.
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.
👍
@ -487,3 +488,2 @@
export async function getAuthForRoute(route: RouteLocation) {
const authStore = useAuthStore()
export async function getAuthForRoute(to: RouteLocation, authStore) {
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
I think this should be possible now with vue-router 4.2.0. See
909c45be19
@dpschen Can you check this again?
e3e4b8a563
to34182b8bbb