Copilot commented on code in PR #11812: URL: https://github.com/apache/gravitino/pull/11812#discussion_r3503201144
########## web/web/src/app/login/components/BasicLogin.js: ########## @@ -0,0 +1,138 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +'use client' + +import { useRouter } from 'next/navigation' +import { useEffect } from 'react' +import { Grid, Button, TextField, FormControl, FormHelperText } from '@mui/material' +import * as yup from 'yup' +import { useForm, Controller } from 'react-hook-form' +import { yupResolver } from '@hookform/resolvers/yup' + +import { useAppDispatch, useAppSelector } from '@/lib/hooks/useStore' +import { basicLoginAction, setIntervalIdAction, clearIntervalId } from '@/lib/store/auth' + +const defaultValues = { + username: '', + password: '' +} + +const schema = yup.object().shape({ + username: yup.string().required(), + password: yup.string().required() +}) + +function BasicLogin() { + const router = useRouter() + const dispatch = useAppDispatch() + const store = useAppSelector(state => state.auth) + + const { + control, + handleSubmit, + reset, + setError, + formState: { errors } + } = useForm({ + defaultValues: Object.assign({}, defaultValues), + mode: 'onChange', + resolver: yupResolver(schema) + }) + + useEffect(() => { + if (store.intervalId) { + dispatch(clearIntervalId()) + } + }, [store.intervalId, dispatch]) + + const onSubmit = async data => { + try { + //Using .unwrap() to catch failed login errors + await dispatch(basicLoginAction({ username: data.username, password: data.password, router })).unwrap() + await dispatch(setIntervalIdAction()) + + reset({ ...data }) Review Comment: basicLoginAction uses a static Basic credential (no expiry), but this component starts the refresh-token interval via setIntervalIdAction(). That interval calls refreshToken() (OAuth flow) and will keep firing in basic mode, causing repeated failing requests/toasts and potentially clearing auth state. Also, after successful login the form is reset to the submitted data, leaving the password in the input state; it should be cleared. ########## web/web/src/app/login/components/BasicLogin.js: ########## @@ -0,0 +1,138 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +'use client' + +import { useRouter } from 'next/navigation' +import { useEffect } from 'react' +import { Grid, Button, TextField, FormControl, FormHelperText } from '@mui/material' +import * as yup from 'yup' +import { useForm, Controller } from 'react-hook-form' +import { yupResolver } from '@hookform/resolvers/yup' + +import { useAppDispatch, useAppSelector } from '@/lib/hooks/useStore' +import { basicLoginAction, setIntervalIdAction, clearIntervalId } from '@/lib/store/auth' + +const defaultValues = { + username: '', + password: '' +} + +const schema = yup.object().shape({ + username: yup.string().required(), + password: yup.string().required() +}) + +function BasicLogin() { + const router = useRouter() + const dispatch = useAppDispatch() + const store = useAppSelector(state => state.auth) + + const { + control, + handleSubmit, + reset, + setError, + formState: { errors } + } = useForm({ + defaultValues: Object.assign({}, defaultValues), + mode: 'onChange', + resolver: yupResolver(schema) + }) + + useEffect(() => { + if (store.intervalId) { + dispatch(clearIntervalId()) + } + }, [store.intervalId, dispatch]) + + const onSubmit = async data => { + try { + //Using .unwrap() to catch failed login errors + await dispatch(basicLoginAction({ username: data.username, password: data.password, router })).unwrap() + await dispatch(setIntervalIdAction()) + + reset({ ...data }) + } catch (error) { + // Insert UI error here + setError('password', { + type: 'manual', + message: 'Invalid username or password' + }) + } Review Comment: The catch block always sets "Invalid username or password" on the password field, even for network/server errors thrown by basicLoginAction (e.g., connection refused). This can mislead users and hide the actual failure reason that basicLoginAction already computed. ########## web/web/src/lib/utils/axios/index.js: ########## @@ -213,16 +217,23 @@ const transform = { const errorMessageMode = originConfig?.requestOptions?.errorMessageMode || 'none' const msg = response?.data?.error?.message ?? response?.data?.message ?? '' const err = error?.toString?.() ?? '' + + const isFailedWebUILoginRequest = + response?.status === 401 && + originConfig?.url === '/api/version' && + originConfig?.headers?.['X-Gravitino-Web-Login'] === 'true' + Review Comment: isFailedWebUILoginRequest relies on originConfig.headers['X-Gravitino-Web-Login'], but Axios may normalize header names (e.g. to lowercase) or store headers as an AxiosHeaders instance. This can cause the flag to be missed and fall into the normal 401 redirect flow, reloading the page instead of staying on the login form. ########## server-common/src/main/java/org/apache/gravitino/server/authentication/AuthenticationFilter.java: ########## @@ -41,15 +41,22 @@ import org.apache.gravitino.utils.PrincipalUtils; public class AuthenticationFilter implements Filter { - - private final List<Authenticator> filterAuthenticators; - /** * The matcher used to identify health check paths that bypass authentication. Subclasses may * replace this with a server-specific matcher (e.g. {@code IcebergHealthCheckPathMatcher}). */ protected HealthCheckPathMatcher healthCheckMatcher = new HealthCheckPathMatcher(); + private final List<Authenticator> filterAuthenticators; + + private static final String WEB_LOGIN_HEADER = "X-Gravitino-Web-Login"; + Review Comment: Class member ordering: static constants should be declared before instance fields (per the project member-order guideline). WEB_LOGIN_HEADER is currently after instance fields, which will fail style/checkstyle expectations if enforced consistently. ########## web/web/src/lib/auth/providers/factory.js: ########## @@ -65,6 +65,18 @@ class OAuthProviderFactory { } const config = await response.json() + const authenticators = config['gravitino.authenticators'] || [] + + // Authenticator logic + if (authenticators.includes('org.apache.gravitino.idp.auth.BasicAuthenticator')) { + // If BasicAuthenticator is present, set provider type to 'basic' and do not initialize any OAuth provider + this.providerType = 'basic' + this.currentProvider = null + + return null + } Review Comment: The new BasicAuthenticator detection branch (providerType='basic' and currentProvider=null) isn't covered by existing OAuthProviderFactory tests. Adding a unit test for this path (including getProviderType() and getAccessToken() behavior) would ensure basic mode doesn't regress while evolving OAuth logic. ########## web/web/src/lib/store/auth/index.js: ########## @@ -91,6 +91,36 @@ export const loginAction = createAsyncThunk('auth/loginAction', async ({ params, return { token: access_token, expired: expires_in } }) +export const basicLoginAction = createAsyncThunk( + 'auth/basicLoginAction', + async ({ username, password, router }, { dispatch }) => { + const basicToken = `Basic ${btoa(`${username}:${password}`)}` + + const [err, res] = await to(basicLoginApi(basicToken)) + + if (err || !res) { + const message = + err?.response?.status === 401 ? 'Invalid username or password' : err?.response?.data?.err || err?.message + + toast.error(message, { + id: `global_error_message_status_${err?.response?.status}` + }) + + throw new Error(message) + } + + localStorage.setItem('accessToken', basicToken) + localStorage.setItem('isIdle', false) + localStorage.removeItem('expiredIn') // Basic auth does not have an expiration time Review Comment: basicLoginAction persists a Basic Authorization header (base64(username:password)) into localStorage. Unlike an OAuth access token, this is effectively the user's long-lived credentials and will survive browser restarts; storing it in localStorage significantly increases impact of any XSS or local profile compromise. Consider using sessionStorage (clears on tab close) or a server-issued short-lived token/cookie instead of persisting the Basic credential. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
