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
Owner

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.

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.
konrad added 1 commit 2023-04-01 10:18:26 +00:00
continuous-integration/drone/pr Build is passing Details
9c81a57791
feat: persist link share auth rule in url hash
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.
konrad requested review from dpschen 2023-04-01 10:18:30 +00:00
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://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!

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://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! > Beep boop, I'm a bot.
Member

Wow that was fast!

Wow that was fast!
Author
Owner

Thanks! I felt like it wouldn't be too complicated so I decided on a whim to implement it :D

Thanks! I felt like it wouldn't be too complicated so I decided on a whim to implement it :D
dpschen approved these changes 2023-04-01 11:24:54 +00:00
@ -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.

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()`
Author
Owner

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
dpschen marked this conversation as resolved
@ -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?

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?
Author
Owner

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.
dpschen marked this conversation as resolved
@ -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.

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.
Author
Owner

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
dpschen marked this conversation as resolved
@ -59,6 +61,7 @@ function useAuth() {
const password = ref('')
const authLinkShare = computed(() => authStore.authLinkShare)
const linkShareHashPrefix = '#linkshare='

Import from router

Import from router
Author
Owner

I've moved it to a separate helper file instead and imported that.

I've moved it to a separate helper file instead and imported that.

Can you move that to the constants folder instead?

Can you move that to the `constants` folder instead?
Author
Owner

Done.

Done.
dpschen marked this conversation as resolved
@ -91,0 +93,4 @@
const hash = linkShareHashPrefix + route.params.share
const last = getRedirectRoute()
if (last) {

Return the push here and below

Return the push here and below
Author
Owner

Done

Done
dpschen marked this conversation as resolved
dpschen reviewed 2023-04-01 11:26:43 +00:00
@ -530,0 +534,4 @@
to.hash = from.hash
}
if (to.hash.startsWith(linkShareHashPrefix)) {

Reduce nesting:

if (to.hash.startsWith(linkShareHashPrefix) && getToken() === null) {
// [...]
Reduce nesting: ``` if (to.hash.startsWith(linkShareHashPrefix) && getToken() === null) { // [...] ```
Author
Owner

Done.

Done.
dpschen marked this conversation as resolved
konrad added 1 commit 2023-04-01 12:13:03 +00:00
continuous-integration/drone/pr Build is passing Details
6bf378ec90
chore: rename getRedirectRoute
konrad added 2 commits 2023-04-01 12:17:10 +00:00
konrad added 1 commit 2023-04-01 12:18:43 +00:00
continuous-integration/drone/pr Build is passing Details
a7fdec956f
fix: return redirect
konrad added 1 commit 2023-04-01 12:19:31 +00:00
continuous-integration/drone/pr Build was killed Details
e8cb7c1083
chore: reduce nesting
konrad requested review from dpschen 2023-04-01 12:20:06 +00:00
konrad added 1 commit 2023-04-01 13:17:20 +00:00
continuous-integration/drone/pr Build was killed Details
56396643dd
chore: move const
dpschen reviewed 2023-04-01 15:11:46 +00:00
@ -8,3 +8,3 @@
function redirectIfSaved() {
function getLastVisitedRoute() {
const last = getLastVisited()
if (last !== null) {

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 } ```
Author
Owner

Done.

Done.
dpschen marked this conversation as resolved
dpschen reviewed 2023-04-01 15:12:12 +00:00
@ -18,0 +21,4 @@
function redirectIfSaved() {
const lastRoute = getLastVisitedRoute()
if (lastRoute) {

Same here.

Same here.
Author
Owner

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'
Author
Owner

Changed it again, please check

Changed it again, please check
dpschen marked this conversation as resolved
dpschen reviewed 2023-04-01 15:14:01 +00:00
@ -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 of LINK_SHARE_HASH_PREFIX).

Unsure: Does `route.hash` contain `#` and `=`? Because it's part of `LINK_SHARE_HASH_PREFIX)`.
Author
Owner

From my tests, it contains everything, including # and =.

From my tests, it contains everything, including `#` and `=`.

Okay = makes sense. I was not aware of # though.
Something learnt 🙂.

Okay `=` makes sense. I was not aware of `#` though. Something learnt 🙂.
dpschen marked this conversation as resolved
konrad added 1 commit 2023-04-01 15:16:56 +00:00
continuous-integration/drone/pr Build was killed Details
007282b758
chore: follow the happy path
dpschen reviewed 2023-04-01 15:22:40 +00:00
@ -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 the baseStore.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?

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 the [`baseStore.ready` line](https://kolaente.dev/vikunja/frontend/src/commit/56396643dd1089dbb1a29a50b4f33db08aec60b6/src/router/index.ts#L499). 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:

if (authStore.authUser || authStore.authLinkShare) {
	return
}

Note the check for authLinkShare. This seems like a duplication, because we check the same info in:

if (to.hash.startsWith(linkShareHashPrefix) && getToken() === null) {
In this context I also stumbled over [these lines](https://kolaente.dev/vikunja/frontend/src/commit/56396643dd1089dbb1a29a50b4f33db08aec60b6/src/router/index.ts#L492-L494) in the router: ```ts if (authStore.authUser || authStore.authLinkShare) { return } ``` Note the check for `authLinkShare`. This seems like a duplication, because we check the same info in: ```ts if (to.hash.startsWith(linkShareHashPrefix) && getToken() === null) { ```
Author
Owner

This might be tricky right now because of the baseStore.ready line.

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.

> This might be tricky right now because of the baseStore.ready line. 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.
Author
Owner

I think the problem here is how getAuthForRoute is called in the ready wrapper as well.

I think the problem here is how `getAuthForRoute` is called in the ready wrapper as well.

I'll try aswell

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.

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.
Author
Owner

Then maybe let's move refactoring that to another PR

Then maybe let's move refactoring that to another PR
dpschen reviewed 2023-04-01 15:23:21 +00:00
@ -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 think we should read here `authStore.authUser` instead.
Author
Owner

I'm now using authStore.authLinkShare and that seems to work great. Why would you use authStore.authUser?

I'm now using `authStore.authLinkShare` and that seems to work great. Why would you use `authStore.authUser`?

I'm now using authStore.authLinkShare and that seems to work great.

Ahh yes, that makes even more sense!

> I'm now using `authStore.authLinkShare` and that seems to work great. Ahh yes, that makes even more sense!
dpschen marked this conversation as resolved
dpschen reviewed 2023-04-01 15:31:20 +00:00
dpschen reviewed 2023-04-01 15:36:54 +00:00
@ -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.

This would also pass a hash that doesn't start with `LINK_SHARE_HASH_PREFIX`.
Author
Owner

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.

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.
dpschen reviewed 2023-04-01 15:37:11 +00:00
@ -530,0 +552,4 @@
}
if(!to.fullPath.endsWith(to.hash)) {
return to.fullPath + to.hash

Same here.

Same here.
konrad added 1 commit 2023-04-01 15:56:41 +00:00
continuous-integration/drone/pr Build is passing Details
2d66393874
feat: check link share auth from store instead
dpschen reviewed 2023-04-04 08:42:50 +00:00
@ -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

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.
Author
Owner

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=. 👍
konrad added 1 commit 2023-04-04 09:35:51 +00:00
continuous-integration/drone/pr Build is passing Details
367459edd2
feat: change the link share hash name
konrad added 1 commit 2023-04-11 16:40:34 +00:00
continuous-integration/drone/pr Build is passing Details
e3e4b8a563
fix: follow the happy path
dpschen reviewed 2023-04-12 13:18:34 +00:00
@ -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)?

Out of interest: Why did you pass the authStore (makes usage e.g. in ready.vue more complex)?
Author
Owner

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
Author
Owner

@dpschen Can you check this again?

@dpschen Can you check this again?
konrad force-pushed feature/persist-link-share-hash-in-url from e3e4b8a563 to 34182b8bbb 2023-06-19 13:28:15 +00:00 Compare
konrad merged commit da3eaf0d35 into main 2023-06-19 14:26:35 +00:00
konrad deleted branch feature/persist-link-share-hash-in-url 2023-06-19 14:26:35 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.