From c6f5b62ad04d0df7354b41b01f1cf0521fc4068a Mon Sep 17 00:00:00 2001 From: Carter <35710697+cmintey@users.noreply.github.com> Date: Thu, 18 Apr 2024 19:17:45 -0500 Subject: [PATCH] Fix OIDC infinite loop if user is not in OIDC_USER_GROUP (#3487) --- frontend/pages/login.vue | 2 +- .../schemes/DynamicOpenIDConnectScheme.js | 2 +- .../security/providers/openid_provider.py | 25 ++++++++------- tests/e2e/docker/docker-compose.yml | 1 + tests/e2e/login.spec.ts | 31 ++++++++++++++++--- 5 files changed, 44 insertions(+), 17 deletions(-) diff --git a/frontend/pages/login.vue b/frontend/pages/login.vue index d0110763ce0a..a3a0b832d0a8 100644 --- a/frontend/pages/login.vue +++ b/frontend/pages/login.vue @@ -201,7 +201,7 @@ export default defineComponent({ } function isDirectLogin() { - return router.currentRoute.query.direct + return Object.keys(router.currentRoute.query).includes("direct") } async function oidcAuthenticate() { diff --git a/frontend/schemes/DynamicOpenIDConnectScheme.js b/frontend/schemes/DynamicOpenIDConnectScheme.js index ee5ec3a19148..4cbc56c8dd60 100644 --- a/frontend/schemes/DynamicOpenIDConnectScheme.js +++ b/frontend/schemes/DynamicOpenIDConnectScheme.js @@ -16,7 +16,6 @@ export default class DynamicOpenIDConnectScheme extends OpenIDConnectScheme { this.$auth.$storage ) - // eslint-disable-next-line @typescript-eslint/no-unsafe-return return await super.mounted() } @@ -79,6 +78,7 @@ export default class DynamicOpenIDConnectScheme extends OpenIDConnectScheme { // Update tokens with mealie token this.updateTokens(response) } catch { + this.$auth.reset() const currentUrl = new URL(window.location.href) if (currentUrl.pathname === "/login" && currentUrl.searchParams.has("direct")) { return diff --git a/mealie/core/security/providers/openid_provider.py b/mealie/core/security/providers/openid_provider.py index c9b157f1abe2..77e1927472f1 100644 --- a/mealie/core/security/providers/openid_provider.py +++ b/mealie/core/security/providers/openid_provider.py @@ -35,17 +35,19 @@ class OpenIDProvider(AuthProvider[OIDCRequest]): repos = get_repositories(self.session) user = self.try_get_user(claims.get(settings.OIDC_USER_CLAIM)) - group_claim = claims.get("groups", []) - is_admin = settings.OIDC_ADMIN_GROUP in group_claim if settings.OIDC_ADMIN_GROUP else False - is_valid_user = settings.OIDC_USER_GROUP in group_claim if settings.OIDC_USER_GROUP else True + is_admin = False + if settings.OIDC_USER_GROUP or settings.OIDC_ADMIN_GROUP: + group_claim = claims.get("groups", []) + is_admin = settings.OIDC_ADMIN_GROUP in group_claim if settings.OIDC_ADMIN_GROUP else False + is_valid_user = settings.OIDC_USER_GROUP in group_claim if settings.OIDC_USER_GROUP else True - if not is_valid_user: - self._logger.debug( - "[OIDC] User does not have the required group. Found: %s - Required: %s", - group_claim, - settings.OIDC_USER_GROUP, - ) - return None + if not is_valid_user: + self._logger.debug( + "[OIDC] User does not have the required group. Found: %s - Required: %s", + group_claim, + settings.OIDC_USER_GROUP, + ) + return None if not user: if not settings.OIDC_SIGNUP_ENABLED: @@ -68,7 +70,7 @@ class OpenIDProvider(AuthProvider[OIDCRequest]): return self.get_access_token(user, settings.OIDC_REMEMBER_ME) # type: ignore if user: - if user.admin != is_admin: + if settings.OIDC_ADMIN_GROUP and user.admin != is_admin: self._logger.debug(f"[OIDC] {'Setting' if is_admin else 'Removing'} user as admin") user.admin = is_admin repos.users.update(user.id, user) @@ -91,6 +93,7 @@ class OpenIDProvider(AuthProvider[OIDCRequest]): self._logger.error( f"[OIDC] Unsupported algorithm '{algorithm}'. Unable to decode id token due to mismatched algorithm." ) + return None try: claims.validate() diff --git a/tests/e2e/docker/docker-compose.yml b/tests/e2e/docker/docker-compose.yml index ca1bc72eb922..ed52317d3830 100644 --- a/tests/e2e/docker/docker-compose.yml +++ b/tests/e2e/docker/docker-compose.yml @@ -30,6 +30,7 @@ services: OIDC_AUTH_ENABLED: True OIDC_SIGNUP_ENABLED: True + OIDC_USER_GROUP: user OIDC_ADMIN_GROUP: admin OIDC_CONFIGURATION_URL: http://localhost:8080/default/.well-known/openid-configuration OIDC_CLIENT_ID: default diff --git a/tests/e2e/login.spec.ts b/tests/e2e/login.spec.ts index f225bcaa3cb7..be2831e674de 100644 --- a/tests/e2e/login.spec.ts +++ b/tests/e2e/login.spec.ts @@ -55,7 +55,8 @@ test('oidc initial login', async ({ page }) => { "sub": username, "email": `${username}@example.com`, "preferred_username": username, - "name": name + "name": name, + "groups": ["user"] } await page.goto('http://localhost:9000/login'); @@ -67,6 +68,26 @@ test('oidc initial login', async ({ page }) => { await expect(page.getByRole('link', { name: 'Settings' })).not.toBeVisible(); }); +test('oidc login with user not in propery group', async ({ page }) => { + const username = "testUserNoGroup" + const name = "Test User No Group" + const claims = { + "sub": username, + "email": `${username}@example.com`, + "preferred_username": username, + "name": name, + "groups": [] + } + + await page.goto('http://localhost:9000/login'); + await page.getByRole('button', { name: 'Login with OAuth' }).click(); + await page.getByPlaceholder('Enter any user/subject').fill(username); + await page.getByPlaceholder('Optional claims JSON value,').fill(JSON.stringify(claims)); + await page.getByRole('button', { name: 'Sign-in' }).click(); + await expect(page).toHaveURL(/.*\/login\/?\?direct=1/) + await expect(page.getByRole('button', { name: 'Login with OAuth' })).toBeVisible() +}); + test('oidc sequential login', async ({ page }) => { const username = "testUser2" const name = "Test User 2" @@ -74,7 +95,8 @@ test('oidc sequential login', async ({ page }) => { "sub": username, "email": `${username}@example.com`, "preferred_username": username, - "name": name + "name": name, + "groups": ["user"] } await page.goto('http://localhost:9000/login'); @@ -100,7 +122,8 @@ test('settings page verify oidc', async ({ page }) => { "sub": username, "email": `${username}@example.com`, "preferred_username": username, - "name": name + "name": name, + "groups": ["user"] } await page.goto('http://localhost:9000/login'); @@ -133,7 +156,7 @@ test('oidc admin user', async ({ page }) => { "email": `${username}@example.com`, "preferred_username": username, "name": name, - "groups": ["admin"] + "groups": ["user", "admin"] } await page.goto('http://localhost:9000/login');