-
Notifications
You must be signed in to change notification settings - Fork 499
console: surface OIDC sign-in failures on the login page #36193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| // Copyright Materialize, Inc. and contributors. All rights reserved. | ||
| // | ||
| // Use of this software is governed by the Business Source License | ||
| // included in the LICENSE file. | ||
| // | ||
| // As of the Change Date specified in that file, in accordance with | ||
| // the Business Source License, use of this software will be governed | ||
| // by the Apache License, Version 2.0. | ||
|
|
||
| import { Heading, HStack, Text, VStack } from "@chakra-ui/react"; | ||
| import React from "react"; | ||
| import { Link as RouterLink } from "react-router-dom"; | ||
|
|
||
| import { LOGIN_PATH } from "~/api/materialize/auth"; | ||
| import Alert from "~/components/Alert"; | ||
| import LoadingScreen from "~/components/LoadingScreen"; | ||
| import { MaterializeLogo } from "~/components/MaterializeLogo"; | ||
| import { useAuth } from "~/external-library-wrappers/oidc"; | ||
| import { AuthContentContainer, AuthLayout } from "~/layouts/AuthLayout"; | ||
|
|
||
| // Surfaces OIDC callback errors (access_denied, state mismatch, etc.) | ||
| export const OidcCallback = () => { | ||
| const auth = useAuth(); | ||
|
|
||
| if (!auth.error) return <LoadingScreen />; | ||
|
|
||
| return ( | ||
| <AuthLayout> | ||
| <AuthContentContainer> | ||
| <VStack alignItems="stretch" width="100%" mx="12"> | ||
| <HStack my={{ base: "8", lg: "0" }} paddingBottom="8"> | ||
| <MaterializeLogo height="12" /> | ||
| </HStack> | ||
| <VStack alignItems="stretch" spacing="2" paddingBottom="6"> | ||
| <Heading size="md">Sign-in failed</Heading> | ||
| <Text fontSize="sm"> | ||
| We couldn't complete your sign-in with your identity | ||
| provider. If this keeps happening, confirm you've been | ||
| granted access to Materialize. | ||
| </Text> | ||
| </VStack> | ||
| <Alert | ||
| variant="error" | ||
| message={auth.error.message || "Sign-in failed. Please try again."} | ||
| showButton | ||
| buttonText="Back to sign in" | ||
| buttonProps={{ as: RouterLink, to: LOGIN_PATH, replace: true }} | ||
| /> | ||
| </VStack> | ||
| </AuthContentContainer> | ||
| </AuthLayout> | ||
| ); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| // Copyright Materialize, Inc. and contributors. All rights reserved. | ||
| // | ||
| // Use of this software is governed by the Business Source License | ||
| // included in the LICENSE file. | ||
| // | ||
| // As of the Change Date specified in that file, in accordance with | ||
| // the Business Source License, use of this software will be governed | ||
| // by the Apache License, Version 2.0. | ||
|
|
||
| // `sessionStorage` key used to pass a one-shot message across the redirect | ||
| // that the login page reads and immediately clears. | ||
| export const LOGIN_REDIRECT_MESSAGE_KEY = "mz.loginRedirectMessage"; | ||
|
|
||
| export type LoginReason = "auth_rejected" | "session_expired"; | ||
|
|
||
| export type LoginRedirectMessage = { | ||
| reason: LoginReason; | ||
| detail?: string; | ||
| }; | ||
|
|
||
| const AUTH_REJECTED_SUFFIX = | ||
| " Please try again, or contact your administrator if this persists."; | ||
|
|
||
| export const LOGIN_REASON_MESSAGES: Record<LoginReason, string> = { | ||
| auth_rejected: `Sign-in was rejected.${AUTH_REJECTED_SUFFIX}`, | ||
| session_expired: "Your previous session ended. Please sign in again.", | ||
|
Comment on lines
+24
to
+26
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could just not print an error for session expiration because we don't know definitively that a 401 with no detail is the session expiring. |
||
| }; | ||
|
|
||
| /** Embeds the sanitized server-side detail (e.g. "invalid audience") into the | ||
| * rejected-login copy. */ | ||
| export const buildAuthRejectedMessage = (detail: string | null): string => { | ||
| if (!detail) return LOGIN_REASON_MESSAGES.auth_rejected; | ||
| return `Sign-in was rejected: ${detail}.${AUTH_REJECTED_SUFFIX}`; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| // Copyright Materialize, Inc. and contributors. All rights reserved. | ||
| // | ||
| // Use of this software is governed by the Business Source License | ||
| // included in the LICENSE file. | ||
| // | ||
| // As of the Change Date specified in that file, in accordance with | ||
| // the Business Source License, use of this software will be governed | ||
| // by the Apache License, Version 2.0. | ||
|
|
||
| import { type LoginReason } from "~/platform/auth/constants"; | ||
|
|
||
| // Cap on the forwarded OIDC detail so a malformed 401 body can't bloat the URL. | ||
| const MAX_AUTH_DETAIL_LENGTH = 200; | ||
|
|
||
| const hasBearer = (input: Parameters<typeof fetch>[0]): boolean => | ||
| input instanceof Request && | ||
| (input.headers.get("Authorization")?.startsWith("Bearer ") ?? false); | ||
|
|
||
| /** Reads the sanitized `OidcError::Display` environmentd returns for OIDC 401s. */ | ||
|
Comment on lines
+15
to
+19
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels like a hacky way to differentiate login errors from session expiration. Rather than use local storage, can we encode the error message into the URL (like a search param) then on the login page, just show the error there? |
||
| export const readAuthErrorDetail = async ( | ||
| response: Response, | ||
| ): Promise<string | undefined> => { | ||
| try { | ||
| const body = (await response.clone().text()).trim(); | ||
| if (!body || body === "unauthorized") return undefined; | ||
| return body.slice(0, MAX_AUTH_DETAIL_LENGTH); | ||
| } catch { | ||
| return undefined; | ||
| } | ||
| }; | ||
|
|
||
| // Returns `auth_rejected` only when a Bearer was sent. | ||
| export const formatLoginErrorForOIDC = ( | ||
| input: Parameters<typeof fetch>[0], | ||
| ): LoginReason | undefined => (hasBearer(input) ? "auth_rejected" : undefined); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -854,26 +854,33 @@ pub(crate) enum AuthError { | |
| FailedToUpdateSession, | ||
| #[error("invalid credentials")] | ||
| InvalidCredentials, | ||
| /// Payload is `OidcError`'s sanitized `Display` (no expected-values leaks). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think similar to the pgwire tests in src/environmentd/tests/auth.rs, we should have tests asserting the details for the http errors. |
||
| #[error("{0}")] | ||
| OidcFailed(String), | ||
| } | ||
|
|
||
| impl IntoResponse for AuthError { | ||
| fn into_response(self) -> Response { | ||
| warn!("HTTP request failed authentication: {}", self); | ||
| let mut headers = HeaderMap::new(); | ||
| match self { | ||
| // We omit most detail from the error message we send to the client, to | ||
| // avoid giving attackers unnecessary information. `OidcFailed` is the | ||
| // exception — its payload is a sanitized `OidcError::Display` that the | ||
| // console embeds in the login-page error. | ||
| let body = match &self { | ||
| AuthError::MissingHttpAuthentication { | ||
| include_www_authenticate_header, | ||
| } if include_www_authenticate_header => { | ||
| } if *include_www_authenticate_header => { | ||
| headers.insert( | ||
| http::header::WWW_AUTHENTICATE, | ||
| HeaderValue::from_static("Basic realm=Materialize"), | ||
| ); | ||
| "unauthorized".to_string() | ||
| } | ||
| _ => {} | ||
| AuthError::OidcFailed(message) => message.clone(), | ||
| _ => "unauthorized".to_string(), | ||
| }; | ||
| // We omit most detail from the error message we send to the client, to | ||
| // avoid giving attackers unnecessary information. | ||
| (StatusCode::UNAUTHORIZED, headers, "unauthorized").into_response() | ||
| (StatusCode::UNAUTHORIZED, headers, body).into_response() | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1241,11 +1248,10 @@ async fn auth( | |
| } | ||
| Authenticator::Oidc(oidc) => match creds { | ||
| Some(Credentials::Token { token }) => { | ||
| // Validate JWT token | ||
| let (mut claims, authenticated) = oidc | ||
| .authenticate(&token, None) | ||
| .await | ||
| .map_err(|_| AuthError::InvalidCredentials)?; | ||
| .map_err(|e| AuthError::OidcFailed(e.to_string()))?; | ||
| let name = std::mem::take(&mut claims.user); | ||
| (name, None, authenticated) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should unify the error behavior with errors from the Materialize side and just redirect to the login page with the error message.