feat: manage caldav tokens #1186

Closed
konrad wants to merge 20 commits from feature/caldav-tokens into main
4 changed files with 147 additions and 31 deletions

View File

@ -103,9 +103,16 @@
"disableSuccess": "Two factor authentication was sucessfully disabled."
},
"caldav": {
"title": "Caldav",
"howTo": "You can connect Vikunja to caldav clients to view and manage all tasks from different clients. Enter this url into your client:",
"more": "More information about caldav in Vikunja"
"title": "CalDAV",
konrad marked this conversation as resolved Outdated

casing

casing
"howTo": "You can connect Vikunja to CalDAV clients to view and manage all tasks from different clients. Enter this url into your client:",
"more": "More information about CalDAV in Vikunja",
"tokens": "CalDAV Tokens",
"tokensHowTo": "You can use a CalDAV token to use instead of a password to log in the above endpoint.",
"createToken": "Create a token",
"tokenCreated": "Here is your token: {token}",
"wontSeeItAgain": "Write it down, you won't be able to see it again.",
"mustUseToken": "You need to create a CalDAV token if you want to use CalDAV with a third party client. Use the token as the password.",
"usernameIs": "Your username is: {0}"
},
"avatar": {
"title": "Avatar",
@ -484,7 +491,10 @@
"showMenu": "Show the menu",
"hideMenu": "Hide the menu",
"forExample": "For example:",
"welcomeBack": "Welcome Back!"
"welcomeBack": "Welcome Back!",
"id": "ID",
"created": "Created at",
"actions": "Actions"
},
"input": {
"resetColor": "Reset Color",

15
src/models/caldavToken.js Normal file
View File

@ -0,0 +1,15 @@
import AbstractModel from './abstractModel'
export default class CaldavTokenModel extends AbstractModel {
constructor(data) {
super(data)
this.created = new Date(this.created)
}
defaults() {
konrad marked this conversation as resolved Outdated

Why does the defaults function exist instead of using field declarations (maybe even private)?

Asking because if it would be:

export default class CaldavTokenModel extends AbstractModel {
	id: 0;
	created: null;

	constructor(data) {
		super(data)
		this.created = new Date(this.created)
	}
}

typing would be better. But maybe I miss something.

Why does the defaults function exist instead of using [field declarations](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes#field_declarations) (maybe even private)? Asking because if it would be: ```ts export default class CaldavTokenModel extends AbstractModel { id: 0; created: null; constructor(data) { super(data) this.created = new Date(this.created) } } ``` typing would be better. But maybe I miss something.

I think it only exists because of my lack of js knowledge back a few years ago when I created the models and services :) It should be cleaned up at some point, but for all models at once (which is kinda out of scope for this PR).

I'm pretty sure there's a better way to do this with typescript so that we have proper type hints everywhere.

I think it only exists because of my lack of js knowledge back a few years ago when I created the models and services :) It should be cleaned up at some point, but for all models at once (which is kinda out of scope for this PR). I'm pretty sure there's a better way to do this with typescript so that we have proper type hints everywhere.

I think field declarations didn't exist then.
Why I was asking: I'm trying to fix all ts errors that I have while working on a segment. And if creating these props as field declarations helps ts I guess it would be a manageable effort.

Agree with the rest of the stuff =)

I think field declarations didn't exist then. Why I was asking: I'm trying to fix all ts errors that I have while working on a segment. And if creating these props as field declarations helps ts I guess it would be a manageable effort. Agree with the rest of the stuff =)
return {
id: 0,
created: null,
}
}
}

View File

@ -0,0 +1,25 @@
import {formatISO} from 'date-fns'
import CaldavTokenModel from '../models/caldavToken'
import AbstractService from './abstractService'
export default class CaldavTokenService extends AbstractService {
constructor() {
super({
getAll: '/user/settings/token/caldav',
create: '/user/settings/token/caldav',
delete: '/user/settings/token/caldav/{id}',
})
}
processModel(model) {
return {
konrad marked this conversation as resolved Outdated

Return a new model instead of modifying the existing:

return {
	...model,
    created: formatISO(new Date(model.created)),
}
Return a new model instead of modifying the existing: ```ts return { ...model, created: formatISO(new Date(model.created)), } ```

Done.

Done.
...model,
created: formatISO(new Date(model.created)),
}
}
modelFactory(data) {
return new CaldavTokenModel(data)
}
}

View File

@ -16,40 +16,106 @@
/>
</div>
</div>
<h5 class="mt-5 mb-4 has-text-weight-bold">
konrad marked this conversation as resolved Outdated

This seems to be a headline? => use <hX>

This seems to be a headline? => use `<hX>`
{{ $t('user.settings.caldav.tokens') }}
</h5>
<p>
<a href="https://vikunja.io/docs/caldav/" rel="noreferrer noopener nofollow" target="_blank">
{{ isLocalUser ? $t('user.settings.caldav.tokensHowTo') : $t('user.settings.caldav.mustUseToken') }}
<template v-if="!isLocalUser">
<br/>
<i18n-t keypath="user.settings.caldav.usernameIs">
<strong>{{ username }}</strong>
</i18n-t>
</template>
</p>
<table class="table" v-if="tokens.length > 0">
<tr>
<th>{{ $t('misc.id') }}</th>
<th>{{ $t('misc.created') }}</th>
<th class="has-text-right">{{ $t('misc.actions') }}</th>
</tr>
<tr v-for="tk in tokens" :key="tk.id">
<td>{{ tk.id }}</td>
<td>{{ formatDateShort(tk.created) }}</td>
<td class="has-text-right">
<x-button type="secondary" @click="deleteToken(tk)">
{{ $t('misc.delete') }}
</x-button>
</td>
</tr>
</table>
<Message v-if="newToken" class="mb-4">
{{ $t('user.settings.caldav.tokenCreated', {token: newToken.token}) }}<br/>
{{ $t('user.settings.caldav.wontSeeItAgain') }}
</Message>
<x-button icon="plus" class="mb-4" @click="createToken" :loading="service.loading">
{{ $t('user.settings.caldav.createToken') }}
</x-button>
<p>
<BaseButton :href="CALDAV_DOCS" rel="noreferrer noopener nofollow" target="_blank">
{{ $t('user.settings.caldav.more') }}
</a>
</BaseButton>
</p>
</card>
</template>
<script>
<script lang="ts" setup>
konrad marked this conversation as resolved Outdated

use ts

use ts
import copy from 'copy-to-clipboard'
import {mapState} from 'vuex'
import {CALDAV_DOCS} from '@/urls'
import {computed, ref} from 'vue'
import {useI18n} from 'vue-i18n'
import {useStore} from 'vuex'
export default {
name: 'user-settings-caldav',
data() {
return {
caldavDocsUrl: CALDAV_DOCS,
}
},
mounted() {
this.setTitle(`${this.$t('user.settings.caldav.title')} - ${this.$t('user.settings.title')}`)
},
computed: {
caldavUrl() {
return `${this.$store.getters['config/apiBase']}/dav/principals/${this.userInfo.username}/`
},
...mapState('config', ['caldavEnabled']),
...mapState({
userInfo: state => state.auth.info,
}),
},
methods: {
copy,
},
import {CALDAV_DOCS} from '@/urls'
import {useTitle} from '@/composables/useTitle'
import {success} from '@/message'
import BaseButton from '@/components/base/BaseButton.vue'
konrad marked this conversation as resolved Outdated

use @

use `@`
import Message from '@/components/misc/message.vue'
import CaldavTokenService from '@/services/caldavToken'
import CaldavTokenModel from '@/models/caldavToken'
dpschen marked this conversation as resolved Outdated
  • Move service in useTokens function.
  • Make service a shallowReactive. => Will be destroyed when component is unmounted.
  • return a new computed serviceIsLoading from useTokens that computes the isLoading property.

=> isolated abstraction

- Move service in `useTokens` function. - Make service a `shallowReactive`. => Will be destroyed when component is unmounted. - return a new computed `serviceIsLoading` from `useTokens` that computes the isLoading property. => isolated abstraction

Wouldn't I need to do this for all other functions as well? (deleteToken and createToken) How would I handle loading state from all of them? I'd like to avoid having a bunch of loading variables for each of these functions.

Wouldn't I need to do this for all other functions as well? (`deleteToken` and `createToken`) How would I handle loading state from all of them? I'd like to avoid having a bunch of loading variables for each of these functions.

Solved differently in #1307/

Solved differently in https://kolaente.dev/vikunja/frontend/pulls/1307/
const service = new CaldavTokenService()
dpschen marked this conversation as resolved Outdated

We should call this function then useTokens.


Unsure:

I think : ref<CaldavTokenModel[]> is now unnecessary since the type of the return was already defined by the line:

const tokens = ref<CaldavTokenModel[]>([])

Trying to learn: Not sure what the go-to way for ts is in such a case.

We should call this function then `useTokens`. ---- Unsure: I think `: ref<CaldavTokenModel[]>` is now unnecessary since the type of the return was already defined by the line: ```ts const tokens = ref<CaldavTokenModel[]>([]) ``` Trying to learn: Not sure what the go-to way for ts is in such a case.

Renamed the function.

I'm not quite sure about the other one, since the const token now does not have a type declaration, only the function does. I think that should be fine?

Renamed the function. I'm not quite sure about the other one, since the `const token` now does not have a type declaration, only the function does. I think that should be fine?

I cannot follow you:
In this line the type of the returned value is defined.

I cannot follow you: In [this line](https://kolaente.dev/vikunja/frontend/src/commit/ce479b40769d4a116694b39a4d552e561a84b8d9/src/views/user/settings/Caldav.vue#L85) the type of the returned value is defined.

Regardless this is not important anymore, since the code changed.

Regardless this is not important anymore, since the code changed.
function useTokens(): ref<CaldavTokenModel[]> {
const tokens = ref<CaldavTokenModel[]>([])
dpschen marked this conversation as resolved Outdated

A composable should usually not return a promise. See https://antfu.me/posts/async-with-composition-api
(only exception as far as I know seems to be when you use the <suspense> component)

Better: create a new function inside the composable that does the async action and call it without await.
Something like:

function init() {
	tokens.value = await service.getAll()
}

// and then call it either directly
init()

// or return it so the user of the composable can call it when he wants (probably not what you want here).
return {
	init,
    [...]
}

EDIT:
There is also: useAsyncState

A composable should *usually* not return a promise. See https://antfu.me/posts/async-with-composition-api (only exception as far as I know seems to be when you use the `<suspense>` component) Better: create a new function inside the composable that does the async action and call it without await. Something like: ```ts function init() { tokens.value = await service.getAll() } // and then call it either directly init() // or return it so the user of the composable can call it when he wants (probably not what you want here). return { init, [...] } ``` **EDIT:** There is also: [useAsyncState](https://vueuse.org/core/useasyncstate/)

But afaik I need an async function when using await? Or use .then()?

Using the example you sent gives me an error about that.

But afaik I need an `async` function when using `await`? Or use `.then()`? Using the example you sent gives me an error about that.

Okay so this does not seem to work at all right now.

Okay so this does not seem to work at all right now.

I think I got it working in ef69fd28b2 but I'm not sure if that's a good way to do it. I think I'm returning the ref before it has data in it, that only gets populated once the callback in .then is done and puts it in tokens.value. Not sure if that's a good way to do it?

I think I got it working in ef69fd28b299ad965a9f68e467cee5d166df9e1d but I'm not sure if that's a good way to do it. I think I'm returning the ref before it has data in it, that only gets populated once the callback in `.then` is done and puts it in `tokens.value`. Not sure if that's a good way to do it?

Continuing this is #1307/

Continuing this is https://kolaente.dev/vikunja/frontend/pulls/1307/
service.getAll()
.then((t: CaldavTokenModel[]) => {
tokens.value = t
})
return tokens
konrad marked this conversation as resolved Outdated

Abstract with new useToken composable (can be local) that returns the tokens ref.
Then the getAll call can also be async (an async init function that is called without await).

Abstract with new `useToken` composable (can be local) that returns the tokens ref. Then the getAll call can also be async (an async init function that is called without await).

How do I properly type hint this? I tried this:

async function useToken(): Promise<ref<CaldavTokenModel[]>> {
	const tokens = ref<CaldavTokenModel[]>([])
	tokens.value = await service.getAll()
	return tokens
}

but that does not seem to work.

How do I properly type hint this? I tried this: ```ts async function useToken(): Promise<ref<CaldavTokenModel[]>> { const tokens = ref<CaldavTokenModel[]>([]) tokens.value = await service.getAll() return tokens } ``` but that does not seem to work.

What errors do you get?

I guess not all props are defined (because I got those kind of errors all the time with the models).

For this the field declarations might help

What errors do you get? I guess not all props are defined (because I got those kind of errors all the time with the models). For this the [field declarations](https://kolaente.dev/vikunja/frontend/pulls/1186#issuecomment-21746) might help

Seems like

async function useToken(): ref<CaldavTokenModel[]> {

did the trick.

Seems like ```js async function useToken(): ref<CaldavTokenModel[]> { ``` did the trick.

but that does not seem to work.

Not sure what problem you had here, but I realize now that it might have also been because of the usage of await in a composable.

> but that does not seem to work. Not sure what problem you had here, but I realize now that it might have also been because of the usage of await in a composable.
}
const tokens = useTokens()
const store = useStore()
const {t} = useI18n()
useTitle(() => `${t('user.settings.caldav.title')} - ${t('user.settings.title')}`)
konrad marked this conversation as resolved Outdated

createToken should be an async function (also defined in useToken composable).

`createToken` should be an async function (also defined in `useToken` composable).

Done.

Done.

Use function statement.

=> Makes it more clear that these are the "methods" of vue and makes them stand out more against computeds and refs.

Use `function` statement. => Makes it more clear that these are the "methods" of vue and makes them stand out more against computeds and refs.

Done.

Done.
const caldavUrl = computed(() => `${store.getters['config/apiBase']}/dav/principals/${store.state.auth.info.username}/`)
const caldavEnabled = computed(() => store.state.config.caldavEnabled)
const isLocalUser = computed(() => store.state.auth.info?.isLocalUser)
const username = computed(() => store.state.auth.info?.username)
konrad marked this conversation as resolved Outdated

Would there be any negative side effect if you would assign to newToken.value directly?

newToken.value = await service.create({})
tokens.value.push(newToken.value)

If yes (picky): I prefere to give api returns a name that makes clear what they contain, e.g. token.

Would there be any negative side effect if you would assign to `newToken.value` directly? ```ts newToken.value = await service.create({}) tokens.value.push(newToken.value) ``` If yes (picky): I prefere to give api returns a name that makes clear what they contain, e.g. `token`.

That seems to work fine, I've changed it accordingly.

That seems to work fine, I've changed it accordingly.
const newToken = ref(null)
konrad marked this conversation as resolved Outdated

Should be async function

Should be async function

Done.

Done.

Picky: Use function statement.

Picky: Use function statement.
async function createToken() {
newToken.value = await service.create({})
tokens.value.push(newToken.value)
}
dpschen marked this conversation as resolved Outdated

Use filter

Use filter

Done.

Done.

Now it's findIndex but this seems to have other problems, because of the @ts-ignore.

With something like:

const filteredTokenEntries = Objects.entries(token.value).filter([...])
token.value = Object.fromEntries(filteredTokenEntries)

It might work =)


On the other hand:
Maybe with that the original solution was nicer / smarter.
I somehow thought tokens is an array and not an object… that's why I proposed using filter in the first place.

Now it's `findIndex` but this seems to have other problems, because of the `@ts-ignore`. With something like: ```js const filteredTokenEntries = Objects.entries(token.value).filter([...]) token.value = Object.fromEntries(filteredTokenEntries) ``` It might work =) ----- On the other hand: Maybe with that the original solution was nicer / smarter. I somehow thought tokens is an array and not an object… that's why I proposed using `filter` in the first place.

I somehow thought tokens is an array and not an object… that's why I proposed using filter in the first place.

tokens is an array, not an object. But I need the index of the entry I want to feed into splice to remove the entry. AFAIK I can't use find because that would only give me the object in the array, not the index?

Isn't findIndex pretty much the same as a for(const i in ..) with a break?

> I somehow thought tokens is an array and not an object… that's why I proposed using filter in the first place. `tokens` is an array, not an object. But I need the index of the entry I want to feed into `splice` to remove the entry. AFAIK I can't use `find` because that would only give me the object in the array, not the index? Isn't `findIndex` pretty much the same as a `for(const i in ..)` with a `break`?

It is if you add a condition for the case of -1 (which you did now)

It is if you add a condition for the case of `-1` (which you did now)
konrad marked this conversation as resolved Outdated

Why the @ts-ignore? => Add comment if you need it

Why the `@ts-ignore`? => Add comment if you need it

I just took another look and think I figured out how to fix the types on the useToken function so the @ts-ignore isn't required anymore. Should be fine now.

I just took another look and think I figured out how to fix the types on the `useToken` function so the `@ts-ignore` isn't required anymore. Should be fine now.

Move success after all state changes

Move `success` after all state changes

Done.

Done.
async function deleteToken(token: CaldavTokenModel) {
konrad marked this conversation as resolved Outdated

I think you need to check for -1.

I think you need to check for `-1`.

Done.

Done.
const r = await service.delete(token)
konrad marked this conversation as resolved Outdated

Why the @ts-ignore? => Add comment if you need it

Why the `@ts-ignore`? => Add comment if you need it
const i = tokens.value.findIndex(v => v.id === token.id)
if (i === -1) {
return
}
tokens.value.splice(i, 1)
success(r)
}
</script>