mirror of
https://github.com/go-vikunja/vikunja.git
synced 2026-08-31 12:45:51 -05:00
fix(auth): don't auto-redirect to the OIDC provider when the login URL must stay copyable
The single-provider auto-redirect added in 18ee92f2 fires on every visit to
/login, including the two cases where the login page has to stay put:
- A native client's /oauth/authorize URL is parked in the login hash so it can
be copied into the browser the user is actually signed in to (#2654). The
auto-redirect replaced that URL with the provider's before it could be
copied. Copying the provider URL instead does not work either, because the
OIDC state lives in the originating browser's localStorage, so finishing the
flow elsewhere fails the state check.
- Inside the Electron window, login is handed off to the system browser by
DesktopLogin. Redirecting to the provider in-window stranded the user there.
Move the decision into getAutoRedirectProvider() and cover it with unit tests.
Fixes #3473
This commit is contained in:
@@ -0,0 +1,51 @@
|
||||
import {describe, it, expect} from 'vitest'
|
||||
|
||||
import {getAutoRedirectProvider} from './redirectToProvider'
|
||||
import type {IProvider} from '@/types/IProvider'
|
||||
|
||||
const provider = {key: 'authentik', name: 'Authentik'} as IProvider
|
||||
|
||||
const soleProviderContext = {
|
||||
localAuthEnabled: false,
|
||||
ldapAuthEnabled: false,
|
||||
openIdEnabled: true,
|
||||
providers: [provider],
|
||||
isDesktopApp: false,
|
||||
justLoggedOut: false,
|
||||
hasCopyableRedirect: false,
|
||||
}
|
||||
|
||||
describe('getAutoRedirectProvider', () => {
|
||||
it('returns the provider when it is the only way to log in', () => {
|
||||
expect(getAutoRedirectProvider(soleProviderContext)).toBe(provider)
|
||||
})
|
||||
|
||||
it('does not redirect when the login url carries a copyable oauth destination', () => {
|
||||
expect(getAutoRedirectProvider({...soleProviderContext, hasCopyableRedirect: true})).toBeUndefined()
|
||||
})
|
||||
|
||||
it('does not redirect inside the desktop app', () => {
|
||||
expect(getAutoRedirectProvider({...soleProviderContext, isDesktopApp: true})).toBeUndefined()
|
||||
})
|
||||
|
||||
it('does not redirect right after an explicit logout', () => {
|
||||
expect(getAutoRedirectProvider({...soleProviderContext, justLoggedOut: true})).toBeUndefined()
|
||||
})
|
||||
|
||||
it('does not redirect when local or ldap auth is available', () => {
|
||||
expect(getAutoRedirectProvider({...soleProviderContext, localAuthEnabled: true})).toBeUndefined()
|
||||
expect(getAutoRedirectProvider({...soleProviderContext, ldapAuthEnabled: true})).toBeUndefined()
|
||||
})
|
||||
|
||||
it('does not redirect when there is a choice of providers', () => {
|
||||
expect(getAutoRedirectProvider({
|
||||
...soleProviderContext,
|
||||
providers: [provider, {key: 'other', name: 'Other'} as IProvider],
|
||||
})).toBeUndefined()
|
||||
})
|
||||
|
||||
it('does not redirect when openid is disabled or has no providers', () => {
|
||||
expect(getAutoRedirectProvider({...soleProviderContext, openIdEnabled: false})).toBeUndefined()
|
||||
expect(getAutoRedirectProvider({...soleProviderContext, providers: []})).toBeUndefined()
|
||||
})
|
||||
})
|
||||
@@ -31,3 +31,48 @@ export const redirectToProviderOnLogout = (provider: IProvider): boolean => {
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
interface AutoRedirectContext {
|
||||
localAuthEnabled: boolean
|
||||
ldapAuthEnabled: boolean
|
||||
openIdEnabled: boolean
|
||||
providers: IProvider[]
|
||||
isDesktopApp: boolean
|
||||
justLoggedOut: boolean
|
||||
hasCopyableRedirect: boolean
|
||||
}
|
||||
|
||||
/**
|
||||
* The provider the login page should redirect to without the user clicking anything,
|
||||
* or undefined when it must render the login form instead.
|
||||
*/
|
||||
export function getAutoRedirectProvider(ctx: AutoRedirectContext): IProvider | undefined {
|
||||
// The Electron window hands login off to the system browser via DesktopLogin – redirecting
|
||||
// to the provider in-window would strand the user there with no way back to the app.
|
||||
if (ctx.isDesktopApp) {
|
||||
return undefined
|
||||
}
|
||||
|
||||
// Otherwise we'd immediately re-authenticate the user we just logged out.
|
||||
if (ctx.justLoggedOut) {
|
||||
return undefined
|
||||
}
|
||||
|
||||
// A native client's authorize URL is parked in the login hash so it stays copyable into the
|
||||
// browser the user is actually signed in to (#2654). Redirecting to the provider replaces it
|
||||
// before it can be copied, and the provider URL itself is not transferable: the OIDC state
|
||||
// lives in this browser's localStorage, so finishing the flow elsewhere fails the state check.
|
||||
if (ctx.hasCopyableRedirect) {
|
||||
return undefined
|
||||
}
|
||||
|
||||
if (ctx.localAuthEnabled || ctx.ldapAuthEnabled) {
|
||||
return undefined
|
||||
}
|
||||
|
||||
if (!ctx.openIdEnabled || ctx.providers.length !== 1) {
|
||||
return undefined
|
||||
}
|
||||
|
||||
return ctx.providers[0]
|
||||
}
|
||||
|
||||
@@ -117,7 +117,7 @@
|
||||
<script setup lang="ts">
|
||||
import {computed, onBeforeMount, ref} from 'vue'
|
||||
import {useI18n} from 'vue-i18n'
|
||||
import {useRouter} from 'vue-router'
|
||||
import {useRoute, useRouter} from 'vue-router'
|
||||
import {useDebounceFn} from '@vueuse/core'
|
||||
|
||||
import Message from '@/components/misc/Message.vue'
|
||||
@@ -127,9 +127,10 @@ import FormCheckbox from '@/components/input/FormCheckbox.vue'
|
||||
import DesktopLogin from '@/views/user/DesktopLogin.vue'
|
||||
|
||||
import {getErrorText} from '@/message'
|
||||
import {redirectToProvider} from '@/helpers/redirectToProvider'
|
||||
import {getAutoRedirectProvider, redirectToProvider} from '@/helpers/redirectToProvider'
|
||||
import {useRedirectToLastVisited} from '@/composables/useRedirectToLastVisited'
|
||||
import {isDesktopApp} from '@/helpers/desktopAuth'
|
||||
import {REDIRECT_HASH_PREFIX} from '@/constants/redirectHash'
|
||||
|
||||
import {useAuthStore, JUST_LOGGED_OUT_KEY} from '@/stores/auth'
|
||||
import {useConfigStore} from '@/stores/config'
|
||||
@@ -139,6 +140,7 @@ import {useTitle} from '@/composables/useTitle'
|
||||
const {t} = useI18n({useScope: 'global'})
|
||||
useTitle(() => t('user.auth.login'))
|
||||
|
||||
const route = useRoute()
|
||||
const router = useRouter()
|
||||
const authStore = useAuthStore()
|
||||
const configStore = useConfigStore()
|
||||
@@ -179,22 +181,23 @@ onBeforeMount(() => {
|
||||
return
|
||||
}
|
||||
|
||||
// Don't auto-redirect right after an explicit logout, otherwise we'd
|
||||
// immediately re-authenticate the user we just logged out.
|
||||
if (sessionStorage.getItem(JUST_LOGGED_OUT_KEY)) {
|
||||
// Consumed on read so the next visit to the login page can auto-redirect again.
|
||||
const justLoggedOut = sessionStorage.getItem(JUST_LOGGED_OUT_KEY) !== null
|
||||
if (justLoggedOut) {
|
||||
sessionStorage.removeItem(JUST_LOGGED_OUT_KEY)
|
||||
return
|
||||
}
|
||||
|
||||
// When the login page offers nothing but a single OIDC provider, skip it
|
||||
// and send the user straight there.
|
||||
if (
|
||||
!localAuthEnabled.value &&
|
||||
!ldapAuthEnabled.value &&
|
||||
hasOpenIdProviders.value &&
|
||||
openidConnect.value.providers.length === 1
|
||||
) {
|
||||
redirectToProvider(openidConnect.value.providers[0])
|
||||
const autoRedirectProvider = getAutoRedirectProvider({
|
||||
localAuthEnabled: localAuthEnabled.value,
|
||||
ldapAuthEnabled: ldapAuthEnabled.value,
|
||||
openIdEnabled: openidConnect.value.enabled,
|
||||
providers: openidConnect.value.providers ?? [],
|
||||
isDesktopApp: isDesktop,
|
||||
justLoggedOut,
|
||||
hasCopyableRedirect: route.hash.startsWith(REDIRECT_HASH_PREFIX),
|
||||
})
|
||||
if (autoRedirectProvider) {
|
||||
redirectToProvider(autoRedirectProvider)
|
||||
}
|
||||
})
|
||||
|
||||
|
||||
Reference in New Issue
Block a user