diff --git a/api/oauth2_metadata/constants.py b/api/oauth2_metadata/constants.py index 115e737ef4d5..061dbd5f0e92 100644 --- a/api/oauth2_metadata/constants.py +++ b/api/oauth2_metadata/constants.py @@ -6,3 +6,20 @@ FIRST_PARTY_CLIENT_IDS = frozenset({FLAGSMITH_CLI_CLIENT_ID}) FIRST_PARTY_SCOPES = frozenset({SCOPE_ADMIN_API}) THIRD_PARTY_SCOPES = frozenset({SCOPE_MCP}) + +SCOPE_GRANTS: dict[str, tuple[str, ...]] = { + SCOPE_MCP: ( + "Manage feature flags, toggle states, and update values", + "Create and manage audience targeting segments", + "View and configure environments", + "View and update project settings", + "Create and review change requests", + "View organisation details, roles, and groups", + "Act with your own permissions, in every organisation you belong to", + ), + SCOPE_ADMIN_API: ( + "Act with your own permissions, in every organisation you belong to", + "Manage feature flags, segments, environments and projects you can access", + "Manage organisation settings, members and roles you can access", + ), +} diff --git a/api/oauth2_metadata/mappers.py b/api/oauth2_metadata/mappers.py index 4d10aa009919..fbd30f384c64 100644 --- a/api/oauth2_metadata/mappers.py +++ b/api/oauth2_metadata/mappers.py @@ -1,5 +1,11 @@ +from collections.abc import Iterable from typing import Any +from oauth2_provider.scopes import get_scopes_backend + +from oauth2_metadata.constants import SCOPE_GRANTS +from oauth2_metadata.types import ScopeDescription + _RFC7591_ERROR_CODES: dict[str, str] = { "redirect_uris": "invalid_redirect_uri", "client_name": "invalid_client_metadata", @@ -9,6 +15,25 @@ } +def map_scopes_to_descriptions( + scopes: Iterable[str], +) -> dict[str, ScopeDescription]: + """Describe scopes for a consent screen. + + The label is the scope registry's own one-liner; the grants spell it out. + A scope with nothing written about it is described by its label alone, + so an added scope is never presented as granting nothing. + """ + all_scopes: dict[str, str] = get_scopes_backend().get_all_scopes() + return { + scope: ScopeDescription( + label=all_scopes.get(scope, scope), + grants=list(SCOPE_GRANTS.get(scope, ())), + ) + for scope in scopes + } + + def map_drf_error_to_rfc7591_error_body(errors: dict[str, Any]) -> dict[str, str]: """Format DRF serializer errors per RFC 7591 section 3.2.2.""" first_field = next(iter(errors)) diff --git a/api/oauth2_metadata/types.py b/api/oauth2_metadata/types.py new file mode 100644 index 000000000000..868b2a808018 --- /dev/null +++ b/api/oauth2_metadata/types.py @@ -0,0 +1,8 @@ +from typing import TypedDict + + +class ScopeDescription(TypedDict): + """How a scope is presented to a user asked to consent to it.""" + + label: str + grants: list[str] diff --git a/api/oauth2_metadata/views.py b/api/oauth2_metadata/views.py index a9fb137f368c..2819d2589e80 100644 --- a/api/oauth2_metadata/views.py +++ b/api/oauth2_metadata/views.py @@ -7,7 +7,6 @@ from django.views.decorators.http import require_GET from oauth2_provider.exceptions import OAuthToolkitError from oauth2_provider.models import get_application_model -from oauth2_provider.scopes import get_scopes_backend from oauth2_provider.views.mixins import OAuthLibMixin from rest_framework import status from rest_framework import status as drf_status @@ -18,7 +17,10 @@ from rest_framework.views import APIView from oauth2_metadata.dataclasses import OAuthConfig -from oauth2_metadata.mappers import map_drf_error_to_rfc7591_error_body +from oauth2_metadata.mappers import ( + map_drf_error_to_rfc7591_error_body, + map_scopes_to_descriptions, +) from oauth2_metadata.metrics import flagsmith_oauth2_dcr_registrations_total from oauth2_metadata.serializers import ( TOKEN_ENDPOINT_AUTH_METHODS, @@ -84,15 +86,13 @@ def get(self, request: Request, *args: Any, **kwargs: Any) -> Response: application = Application.objects.get( client_id=credentials["client_id"], ) - all_scopes = get_scopes_backend().get_all_scopes() - scopes_dict: dict[str, str] = {s: all_scopes.get(s, s) for s in scopes} return Response( { "application": { "name": application.name, "client_id": application.client_id, }, - "scopes": scopes_dict, + "scopes": map_scopes_to_descriptions(scopes), "redirect_uri": credentials.get("redirect_uri", ""), # skip_authorization is safe to reuse here: this custom view # always shows the consent screen regardless of this flag. diff --git a/api/tests/unit/oauth2_metadata/test_authorize_view.py b/api/tests/unit/oauth2_metadata/test_authorize_view.py index e5fa19e887a0..0467f5652458 100644 --- a/api/tests/unit/oauth2_metadata/test_authorize_view.py +++ b/api/tests/unit/oauth2_metadata/test_authorize_view.py @@ -9,7 +9,12 @@ from rest_framework import status from rest_framework.test import APIClient -from oauth2_metadata.constants import FLAGSMITH_CLI_CLIENT_ID +from oauth2_metadata.constants import ( + FLAGSMITH_CLI_CLIENT_ID, + SCOPE_ADMIN_API, + SCOPE_GRANTS, + SCOPE_MCP, +) def _pkce_pair() -> tuple[str, str]: @@ -82,7 +87,10 @@ def test_get__valid_params__returns_application_info( data = response.json() assert data["application"]["name"] == "Test App" assert data["application"]["client_id"] == oauth_application.client_id - assert "mcp" in data["scopes"] + assert data["scopes"]["mcp"] == { + "label": "MCP access", + "grants": list(SCOPE_GRANTS[SCOPE_MCP]), + } assert data["redirect_uri"] == "https://example.com/callback" assert data["is_verified"] is False @@ -330,5 +338,8 @@ def test_get__flagsmith_cli_requests_admin_api__returns_application_info( assert response.status_code == status.HTTP_200_OK data = response.json() assert data["application"]["client_id"] == FLAGSMITH_CLI_CLIENT_ID - assert "admin-api" in data["scopes"] + assert data["scopes"]["admin-api"] == { + "label": "Admin API access", + "grants": list(SCOPE_GRANTS[SCOPE_ADMIN_API]), + } assert data["is_verified"] is True diff --git a/api/tests/unit/oauth2_metadata/test_mappers.py b/api/tests/unit/oauth2_metadata/test_mappers.py new file mode 100644 index 000000000000..81a78d18a06f --- /dev/null +++ b/api/tests/unit/oauth2_metadata/test_mappers.py @@ -0,0 +1,66 @@ +from pytest_django.fixtures import SettingsWrapper + +from oauth2_metadata.constants import SCOPE_ADMIN_API, SCOPE_GRANTS, SCOPE_MCP +from oauth2_metadata.mappers import map_scopes_to_descriptions + + +def test_map_scopes_to_descriptions__described_scopes__returns_labels_and_grants() -> ( + None +): + # Given + scopes = [SCOPE_MCP, SCOPE_ADMIN_API] + + # When + descriptions = map_scopes_to_descriptions(scopes) + + # Then + assert descriptions == { + SCOPE_MCP: { + "label": "MCP access", + "grants": list(SCOPE_GRANTS[SCOPE_MCP]), + }, + SCOPE_ADMIN_API: { + "label": "Admin API access", + "grants": list(SCOPE_GRANTS[SCOPE_ADMIN_API]), + }, + } + + +def test_map_scopes_to_descriptions__scope_without_grants__returns_label_only( + settings: SettingsWrapper, +) -> None: + # Given + settings.OAUTH2_PROVIDER = { + **settings.OAUTH2_PROVIDER, + "SCOPES": {**settings.OAUTH2_PROVIDER["SCOPES"], "read": "Read access"}, + } + + # When + descriptions = map_scopes_to_descriptions(["read"]) + + # Then + assert descriptions == {"read": {"label": "Read access", "grants": []}} + + +def test_map_scopes_to_descriptions__unregistered_scope__falls_back_to_its_name() -> ( + None +): + # Given + scopes = ["not-a-scope"] + + # When + descriptions = map_scopes_to_descriptions(scopes) + + # Then + assert descriptions == {"not-a-scope": {"label": "not-a-scope", "grants": []}} + + +def test_map_scopes_to_descriptions__no_scopes__returns_empty() -> None: + # Given + scopes: list[str] = [] + + # When + descriptions = map_scopes_to_descriptions(scopes) + + # Then + assert descriptions == {} diff --git a/frontend/common/stores/account-store.js b/frontend/common/stores/account-store.js index 08b96eea0522..d1878ddc437e 100644 --- a/frontend/common/stores/account-store.js +++ b/frontend/common/stores/account-store.js @@ -266,7 +266,9 @@ const controller = { // never let analytics break the login flow } } - return controller.onLogin() + // Signing up through an identity provider is still a signup, so it + // gets the same onboarding entry decision as `register` makes. + return controller.onLogin(!!res.is_new_user && !API.getInvite()) }) .catch((e) => API.ajaxHandler(store, e)) }, diff --git a/frontend/common/types/responses.ts b/frontend/common/types/responses.ts index 26be29a3cbea..bc0e867ec39c 100644 --- a/frontend/common/types/responses.ts +++ b/frontend/common/types/responses.ts @@ -517,6 +517,11 @@ export type Subscription = { } export type OnboardingVariant = 'control' | 'single_page' +// What consenting to an OAuth scope grants, as the API describes it. +export type OAuthScopeDescription = { + label: string + grants: string[] +} export type Organisation = { id: number name: string @@ -1516,7 +1521,7 @@ export type Res = { } validateOAuthAuthorize: { application: { name: string; client_id: string } - scopes: Record + scopes: Record redirect_uri: string is_verified: boolean } diff --git a/frontend/common/utils/__tests__/pendingAuthorisation.test.ts b/frontend/common/utils/__tests__/pendingAuthorisation.test.ts new file mode 100644 index 000000000000..a39780852761 --- /dev/null +++ b/frontend/common/utils/__tests__/pendingAuthorisation.test.ts @@ -0,0 +1,41 @@ +import { isPendingAuthorisation } from 'common/utils/pendingAuthorisation' + +describe('isPendingAuthorisation', () => { + it('matches a consent request with its query', () => { + expect( + isPendingAuthorisation( + '/oauth/authorize?client_id=flagsmith-cli&scope=admin-api&state=abc', + ), + ).toBe(true) + }) + + it('matches the trailing-slash form the CLI is sent to', () => { + expect(isPendingAuthorisation('/oauth/authorize/?client_id=x')).toBe(true) + }) + + it('matches the bare path', () => { + expect(isPendingAuthorisation('/oauth/authorize')).toBe(true) + }) + + it('does not match an identity provider callback', () => { + expect(isPendingAuthorisation('/oauth/google?code=abc')).toBe(false) + }) + + it('does not match another page', () => { + expect(isPendingAuthorisation('/project/1/environment/abc/features')).toBe( + false, + ) + }) + + it('does not match an absolute url wearing the path', () => { + expect(isPendingAuthorisation('https://evil.example/oauth/authorize')).toBe( + false, + ) + }) + + it('is false when there is no redirect', () => { + expect(isPendingAuthorisation(undefined)).toBe(false) + expect(isPendingAuthorisation(null)).toBe(false) + expect(isPendingAuthorisation('')).toBe(false) + }) +}) diff --git a/frontend/common/utils/pendingAuthorisation.ts b/frontend/common/utils/pendingAuthorisation.ts new file mode 100644 index 000000000000..b08b8fd410d6 --- /dev/null +++ b/frontend/common/utils/pendingAuthorisation.ts @@ -0,0 +1,18 @@ +// The dashboard hosts the OAuth consent screen (it is the authorization +// endpoint in our RFC 8414 metadata), so a CLI or MCP client that opened it +// while logged out is now blocked on a loopback callback that only this browser +// can answer - and it gives up after a few minutes. +export const AUTHORISE_PATH = '/oauth/authorize' + +/** + * Whether a stored post-login redirect is a consent request waiting to be + * answered. Compares the whole path so an absolute URL never matches: the + * redirect is read from a cookie, and this decides where we send the browser. + */ +export const isPendingAuthorisation = ( + redirect?: string | null, +): redirect is string => { + if (!redirect) return false + const path = redirect.split('?')[0] + return path.replace(/\/+$/, '') === AUTHORISE_PATH +} diff --git a/frontend/web/components/App.js b/frontend/web/components/App.js index 4faaf15696a6..ebe74ca615a8 100644 --- a/frontend/web/components/App.js +++ b/frontend/web/components/App.js @@ -17,6 +17,7 @@ import { getStoredOnboardingVariant, persistOnboardingEntry, } from 'common/utils/onboardingEntry' +import { AUTHORISE_PATH } from 'common/utils/pendingAuthorisation' import { Provider } from 'react-redux' import { getStore } from 'common/store' import ConfigProvider from 'common/providers/ConfigProvider' @@ -140,6 +141,17 @@ const App = class extends Component { return } + // The consent screen is a destination, not a stop on the way to one, so + // never redirect away from it. Creating an organisation re-fires this while + // the organisation list is still refreshing, and the branch below would + // then yank the user off a request they were about to authorise. + if (this.props.location.pathname.startsWith(AUTHORISE_PATH)) { + return + } + + // A signup with a consent request waiting keeps its redirect cookie + // through the branch below: onboarding provisions the organisation the + // client needs, then answers the request itself. if (!AccountStore.getOrganisation() && !invite) { // New users with no organisation go through the single-page onboarding // flow when it's enabled - it creates the organisation itself, so it diff --git a/frontend/web/components/pages/CreateOrganisationPage.tsx b/frontend/web/components/pages/CreateOrganisationPage.tsx index f78e812ed7ab..08a58bd3092d 100644 --- a/frontend/web/components/pages/CreateOrganisationPage.tsx +++ b/frontend/web/components/pages/CreateOrganisationPage.tsx @@ -9,6 +9,7 @@ import Button from 'components/base/forms/Button' import API from 'project/api' import AppActions from 'common/dispatcher/app-actions' import { getStoredOnboardingTargetingKey } from 'common/utils/onboardingEntry' +import { isPendingAuthorisation } from 'common/utils/pendingAuthorisation' import Utils from 'common/utils/utils' // @ts-ignore import Project from 'common/project' @@ -54,6 +55,17 @@ const CreateOrganisationPage: React.FC = () => { }) } + // A client waiting on the consent screen sent this user here to sign up, + // and the organisation it needed now exists. Answer it before going on: + // it returns the browser to onboarding once the user has authorised. + // Replaced, not pushed - going Back to a spent request only errors. + const pending = API.getRedirect() + if (isPendingAuthorisation(pending)) { + API.setRedirect('') + history.replace(pending) + return + } + history.push('/getting-started') } AccountStore.on('change', onChangeAccountStore) diff --git a/frontend/web/components/pages/OAuthAuthorizePage.tsx b/frontend/web/components/pages/OAuthAuthorizePage.tsx index b8b76642a807..4e573730eba1 100644 --- a/frontend/web/components/pages/OAuthAuthorizePage.tsx +++ b/frontend/web/components/pages/OAuthAuthorizePage.tsx @@ -8,20 +8,6 @@ import Utils from 'common/utils/utils' import Icon from 'components/icons/Icon' import Logo from 'components/Logo' -// Frontend-maintained scope descriptions. The backend returns `mcp` as an -// umbrella scope; we expand it into granular descriptions for the consent UI. -// If a scope is not found here, the backend description is used as fallback. -const SCOPE_DESCRIPTIONS: Record = { - mcp: [ - 'Manage feature flags, toggle states, and update values', - 'Create and manage audience targeting segments', - 'View and configure environments', - 'View and update project settings', - 'Create and review change requests', - 'View organisation details, roles, and groups', - ], -} - const OAuthAuthorizePage = () => { const location = useLocation() const [isRedirecting, setIsRedirecting] = useState(false) @@ -135,25 +121,21 @@ const OAuthAuthorizePage = () => { YOUR ACCOUNT WILL BE USED TO:

+ {/* The API describes each scope: its grants when it has any, + otherwise its label, so a new scope still says something. */} {Object.entries(data.scopes).flatMap(([scope, description]) => { - const descriptions = SCOPE_DESCRIPTIONS[scope] - if (descriptions) { - return descriptions.map((desc, i) => ( -
- - {desc} -
- )) - } - return [ -
+ const items = description.grants?.length + ? description.grants + : [description.label] + return items.map((item, i) => ( +
- {description} -
, - ] + {item} +
+ )) })}
diff --git a/frontend/web/components/pages/home-page/HomePage.tsx b/frontend/web/components/pages/home-page/HomePage.tsx index 1dafc2cc6dbc..a84f3e658ad5 100644 --- a/frontend/web/components/pages/home-page/HomePage.tsx +++ b/frontend/web/components/pages/home-page/HomePage.tsx @@ -228,8 +228,11 @@ const HomePage: React.FC = () => { ) } + // Re-encoded, because fromParam decodes: a redirect carrying its own query + // (an OAuth consent request does) would otherwise be truncated at its first + // `&` as the rest leaked into this page's own params. const redirect = Utils.fromParam().redirect - ? `?redirect=${Utils.fromParam().redirect}` + ? `?redirect=${encodeURIComponent(Utils.fromParam().redirect)}` : '' // Pushed rather than replaced, so Back returns to the signup form with what // was typed still in it. diff --git a/frontend/web/components/pages/onboarding/OnboardingFlow/OnboardingFlow.tsx b/frontend/web/components/pages/onboarding/OnboardingFlow/OnboardingFlow.tsx index 7f8f8acdcfc7..c69406851763 100644 --- a/frontend/web/components/pages/onboarding/OnboardingFlow/OnboardingFlow.tsx +++ b/frontend/web/components/pages/onboarding/OnboardingFlow/OnboardingFlow.tsx @@ -1,4 +1,4 @@ -import React, { FC, useState } from 'react' +import React, { FC, useEffect, useState } from 'react' import { useHistory } from 'react-router-dom' import Button from 'components/base/forms/Button' import Icon from 'components/icons/Icon' @@ -18,6 +18,7 @@ import { useUpdateOrganisationMutation } from 'common/services/useOrganisation' import { useUpdateProjectMutation } from 'common/services/useProject' import API from 'project/api' import Constants from 'common/constants' +import { isPendingAuthorisation } from 'common/utils/pendingAuthorisation' import './OnboardingFlow.scss' type OnboardingSnippet = 'install' | 'wire' @@ -41,6 +42,23 @@ const OnboardingFlow: FC = () => { const [updateOrganisation] = useUpdateOrganisationMutation() const [updateProject] = useUpdateProjectMutation() + // A client waiting on the consent screen sent this user here to sign up. It + // is answered once the workspace exists, never on load: bootstrapping is a + // chain of creates, and navigating away mid-chain leaves it half done. The + // client returns the browser here afterwards, by then with nothing pending. + const [leavingForConsent, setLeavingForConsent] = useState(false) + useEffect(() => { + if (status !== 'ready') { + return + } + const pending = API.getRedirect() + if (isPendingAuthorisation(pending)) { + API.setRedirect('') + setLeavingForConsent(true) + history.replace(pending) + } + }, [status, history]) + // Chromeless flow, so it owns its only exit. const skipToApp = () => history.push( @@ -176,7 +194,7 @@ const OnboardingFlow: FC = () => { } } - if (status === 'creating') { + if (status === 'creating' || leavingForConsent) { return (
diff --git a/frontend/web/main.js b/frontend/web/main.js index b63fa681c807..497004be5a9c 100644 --- a/frontend/web/main.js +++ b/frontend/web/main.js @@ -12,6 +12,7 @@ import Utils from 'common/utils/utils' import Project from 'common/project' import AccountStore from 'common/stores/account-store' import data from 'common/data/base/_data' +import { isPendingAuthorisation } from 'common/utils/pendingAuthorisation' import { openModal, openModal2, @@ -89,9 +90,14 @@ setTimeout(() => { // redirect before login if (!isPublicURL() && !AccountStore.getUser()) { - API.setRedirect( - document.location.pathname + (document.location.search || ''), - ) + // A consent request already waiting outranks the page being loaded. A + // reload part-way through onboarding - which its own Try again performs - + // would otherwise overwrite it and leave the client waiting for ever. + if (!isPendingAuthorisation(API.getRedirect())) { + API.setRedirect( + document.location.pathname + (document.location.search || ''), + ) + } browserHistory.push( `/?redirect=${encodeURIComponent( document.location.pathname + (document.location.search || ''), diff --git a/frontend/web/project/api.ts b/frontend/web/project/api.ts index b3b714b67a14..88e06c90ddfe 100644 --- a/frontend/web/project/api.ts +++ b/frontend/web/project/api.ts @@ -14,6 +14,13 @@ import flagsmith from '@flagsmith/flagsmith' import Utils from 'common/utils/utils' import loadChat, { identifyChatUser } from 'common/loadChat' +// Days, or an absolute date. This is js-cookie's own attribute type, spelled +// out because the package ships no types. +type CookieExpiry = number | Date + +// One hour, in the days js-cookie expects. +const REDIRECT_COOKIE_EXPIRY_DAYS = 1 / 24 + const API = { ajaxHandler( store: { error?: any; goneABitWest: () => void }, @@ -196,7 +203,9 @@ const API = { }, getRedirect(): string | undefined { - return API.getCookie('redirect') + // Read without the refresh getCookie does: a redirect is consumed once, so + // reading it must not extend how long a stale one can fire. + return Cookies.get('redirect') }, getReferrer(): any { @@ -277,12 +286,16 @@ const API = { return flagsmith.logout() }, - setCookie(key: string, v?: string): void { + setCookie( + key: string, + v?: string, + attributes?: { expires?: CookieExpiry }, + ): void { if (!v) { Cookies.remove(key, { domain: Project.cookieDomain, path: '/' }) Cookies.remove(key, { path: '/' }) } else { - const opts = { expires: 30, path: '/' } + const opts = { expires: 30, path: '/', ...attributes } if (!E2E) Object.assign(opts, { sameSite: Project.cookieSameSite || 'none', @@ -305,7 +318,10 @@ const API = { }, setRedirect(v: string): void { - API.setCookie('redirect', v) + // Set moments before a login and consumed moments after. Kept short so an + // abandoned one - a CLI login the user walked away from, say - cannot + // hijack a login weeks later. + API.setCookie('redirect', v, { expires: REDIRECT_COOKIE_EXPIRY_DAYS }) }, trackEvent(data: {