Copilot commented on code in PR #11812:
URL: https://github.com/apache/gravitino/pull/11812#discussion_r3592407904
##########
web-v2/web/src/lib/utils/axios/index.js:
##########
@@ -244,7 +253,7 @@ const transform = {
}
try {
- if (code === 'ECONNABORTED' && message.indexOf('timeout') !== -1) {
+ if (code === 'ECONNABORTED' && message?.indexOf('timeout')) {
Review Comment:
`message?.indexOf('timeout')` is used as a boolean, but `String#indexOf`
returns `-1` when not found (truthy), so this condition will incorrectly treat
*any* ECONNABORTED as a timeout unless `message` is undefined. Use `includes()`
or compare `indexOf(...) !== -1`.
##########
web-v2/web/src/lib/utils/axios/index.js:
##########
@@ -292,8 +312,8 @@ const transform = {
}
const retryRequest = new AxiosRetry()
- const { isOpenRetry } = originConfig.requestOptions.retryRequest
- originConfig.method?.toUpperCase() === RequestEnum.GET && isOpenRetry &&
retryRequest.retry(axiosInstance, error)
+ const { isOpenRetry } = originConfig?.requestOptions?.retryRequest
+ originConfig?.method?.toUpperCase() === RequestEnum.GET && isOpenRetry &&
retryRequest.retry(axiosInstance, error)
Review Comment:
`const { isOpenRetry } = originConfig?.requestOptions?.retryRequest` can
still throw when `retryRequest` is undefined because destructuring from
`undefined` is a runtime error. Read the flag via optional chaining instead.
##########
web-v2/web/src/lib/api/auth/index.js:
##########
@@ -44,3 +44,17 @@ export const loginApi = (url, params) => {
{ withToken: false }
)
}
+
+export const basicLoginApi = basicToken => {
+ return defHttp.get(
+ {
+ url: '/api/version',
+ headers: {
+ Authorization: basicToken,
+ Accept: 'application/vnd.gravitino.v1+json',
+ 'Content-Type': 'application/json'
+ }
Review Comment:
`basicLoginApi` describes using a WebUI login header (per PR description) to
prevent the browser Basic-auth popup, but the request doesn’t currently send
such a header. If the backend gates `WWW-Authenticate: Basic` suppression on a
header, add it here (and drop `Content-Type` for a GET to avoid unnecessary
CORS preflights).
##########
server-common/src/main/java/org/apache/gravitino/server/authentication/AuthenticationFilter.java:
##########
@@ -113,7 +113,9 @@ public void doFilter(ServletRequest request,
ServletResponse response, FilterCha
// to let client to create correct authenticated request.
// Refer to
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/WWW-Authenticate
for (String challenge : ue.getChallenges()) {
- resp.setHeader(AuthConstants.HTTP_CHALLENGE_HEADER, challenge);
+ if (!challenge.toLowerCase().startsWith("basic")) {
+ resp.setHeader(AuthConstants.HTTP_CHALLENGE_HEADER, challenge);
+ }
Review Comment:
This currently suppresses `WWW-Authenticate: Basic` challenges for *all*
clients. The PR description mentions skipping the browser Basic-auth popup via
a **web-login header check**, but no header gating is implemented here, so
non-WebUI clients will also stop receiving Basic challenges on 401. Gate the
suppression on an explicit WebUI header (and add a regression test) so only
WebUI requests skip the Basic challenge.
##########
web-v2/web/src/app/login/page.js:
##########
@@ -42,22 +44,45 @@ const LoginContent = () => {
const maxDurationReason = searchParams.get('reason') === 'max_duration'
const [providerType, setProviderType] = useState(null)
const dispatch = useAppDispatch()
+ const authType = useAppSelector(state => state.auth.authType)
useEffect(() => {
+ dispatch(resetMetalakeStore())
+
+ if (authType !== 'oauth') {
+ return
+ }
+
const detectProviderType = async () => {
try {
const detectedType = await oauthProviderFactory.getProviderType()
setProviderType(detectedType)
} catch (error) {
- setProviderType('default') // fallback to default provider
+ setProviderType('default')
}
}
- dispatch(resetMetalakeStore())
detectProviderType()
- }, [])
+ }, [authType, dispatch])
- const useOidcLogin = providerType === 'oidc'
+ const renderLogin = () => {
+ switch (authType) {
+ case 'basic':
+ return <BasicLogin />
+
+ case 'simple':
+ return <SimpleLogin />
+
+ case 'oauth':
+ if (providerType === null) return null
+ if (providerType === 'oidc') return <OidcLogin />
+
+ return <DefaultLogin />
+
+ default:
+ return null
Review Comment:
When `authType` hasn’t been loaded yet (initially `null` on a fresh visit),
`renderLogin()` returns `null`, resulting in a blank login card until
`/configs` resolves. Show a lightweight loading state instead of rendering
nothing.
##########
web-v2/web/src/lib/store/auth/index.js:
##########
@@ -98,6 +98,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))
Review Comment:
New Basic-login behavior is introduced here (token construction,
`/api/version` probe, sessionStorage persistence, and redirect), but there are
no Jest/unit tests covering the success + 401 failure paths. Given this repo
already has Web UI unit tests (e.g.,
`web-v2/web/src/lib/auth/providers/*.test.js`), please add coverage for
`basicLoginAction` (including that failed login does not trigger a full page
reload).
##########
web-v2/web/src/app/login/components/SimpleLogin.js:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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 { Button, Form, Input } from 'antd'
+
+import { useAppDispatch, useAppSelector } from '@/lib/hooks/useStore'
+import { clearIntervalId, setAuthUser } from '@/lib/store/auth'
+
+function SimpleLogin() {
+ const router = useRouter()
+ const dispatch = useAppDispatch()
+ const store = useAppSelector(state => state.auth)
+ const [form] = Form.useForm()
+
+ useEffect(() => {
+ if (store.intervalId) {
+ clearIntervalId()
+ }
+ }, [store.intervalId])
Review Comment:
`clearIntervalId` is a Redux action creator, but it’s being called directly
here (no dispatch), so the refresh interval will not actually be cleared.
Dispatch the action from the effect.
##########
plugins/idp-basic/src/main/java/org/apache/gravitino/idp/IdpUserGroupManager.java:
##########
@@ -158,7 +158,7 @@ public IdpUser authenticate(String username, String
password) {
"Invalid username or password",
AuthConstants.AUTHORIZATION_BASIC_HEADER.trim());
}
return user;
- } catch (NotFoundException e) {
+ } catch (Exception e) {
throw new UnauthorizedException(
"Invalid username or password",
AuthConstants.AUTHORIZATION_BASIC_HEADER.trim());
}
Review Comment:
Catching `Exception` here will convert unexpected failures (e.g.,
storage/IO/runtime bugs) into a 401 "Invalid username or password", which can
hide real server-side errors and make incidents harder to debug. Limit this
catch to the expected "user not found" case and let other exceptions surface as
internal errors.
--
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]
