Copilot commented on code in PR #10437: URL: https://github.com/apache/gravitino/pull/10437#discussion_r2938034913
########## docs/open-api/authn.yaml: ########## @@ -0,0 +1,71 @@ +# 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. + +--- + +paths: + + /authn/me: + get: + tags: + - authentication + summary: Get the authenticated principal + operationId: getAuthenticatedPrincipal + description: > + Returns the server-resolved principal name for the current authenticated user. + The principal is derived from the JWT token using the configured `principalFields` + and `principalMapper`, ensuring consistency between server-side identity and UI display. + responses: + "200": + description: Returns the authenticated principal information + content: + application/vnd.gravitino.v1+json: + schema: + $ref: "#/components/responses/AuthMeResponse" + examples: Review Comment: The OpenAPI `schema` reference under the 200 response points to `#/components/responses/AuthMeResponse`, but in OpenAPI `schema` must reference a schema object (typically `#/components/schemas/...`). As written, this fragment is not valid OpenAPI and may break spec validation / tooling. ########## docs/open-api/authn.yaml: ########## @@ -0,0 +1,71 @@ +# 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. + +--- + +paths: + + /authn/me: + get: + tags: + - authentication + summary: Get the authenticated principal + operationId: getAuthenticatedPrincipal + description: > + Returns the server-resolved principal name for the current authenticated user. + The principal is derived from the JWT token using the configured `principalFields` + and `principalMapper`, ensuring consistency between server-side identity and UI display. + responses: + "200": + description: Returns the authenticated principal information + content: + application/vnd.gravitino.v1+json: + schema: + $ref: "#/components/responses/AuthMeResponse" + examples: + AuthMeResponse: + $ref: "#/components/examples/AuthMeResponse" + "401": + description: Unauthorized - Authentication credentials are missing or invalid + content: + application/vnd.gravitino.v1+json: + schema: + $ref: "./openapi.yaml#/components/schemas/ErrorModel" + "5xx": + $ref: "./openapi.yaml#/components/responses/ServerErrorResponse" + +components: + responses: + AuthMeResponse: + type: object + properties: + code: + type: integer + format: int32 + description: Status code of the response + enum: + - 0 + principal: + type: string + description: The server-resolved principal name of the authenticated user + Review Comment: `components.responses.AuthMeResponse` is defined as a plain schema object (`type: object`, `properties: ...`). In OpenAPI, `components.responses` entries must be full Response Objects (with `description`/`content`), while reusable schema definitions should live under `components.schemas`. Move this definition to `components.schemas` (or wrap it as a proper response object) to match the existing pattern used in other OpenAPI fragments (e.g., `metalakes.yaml`). ########## web-v2/web/src/lib/auth/providers/oidc.js: ########## @@ -67,17 +67,17 @@ export class OidcOAuthProvider extends BaseOAuthProvider { let user = await this.userManager.getUser() if (user && !user.expired) { - // For JWKS validation, we need the ID token (JWT format), not the access token - return user.id_token || user.access_token + // Use access token for API requests per OAuth2 spec + return user.access_token || user.id_token } if (user && user.expired) { try { // Attempt silent refresh const refreshedUser = await this.userManager.signinSilent() - // Return ID token for JWKS validation - return refreshedUser.id_token || refreshedUser.access_token + // Use access token for API requests per OAuth2 spec + return refreshedUser.access_token || refreshedUser.id_token } catch (refreshError) { Review Comment: This method now prefers `access_token` over `id_token`, but the existing unit test `web-v2/web/src/lib/auth/providers/oidc.test.js` still asserts `id_token` is returned for a valid user and for silent refresh. Update the tests to reflect the new token selection logic; otherwise CI will fail and the behavior change won’t be properly validated. ########## web-v2/web/src/lib/api/auth/index.js: ########## @@ -26,6 +26,12 @@ export const getAuthConfigsApi = () => { }) } +export const getAuthMeApi = () => { + return defHttp.get({ + url: '/api/authn/me' + }) +} Review Comment: PR description says the UI should call `GET /api/auth/me`, but the implemented endpoint (server, docs, and UI client) is `/api/authn/me` (`/authn/me` in OpenAPI with basePath `api`). Please align the PR description and any external references to avoid confusion for API consumers. ########## web-v2/web/src/lib/store/auth/index.js: ########## @@ -102,8 +102,30 @@ export const logoutAction = createAsyncThunk('auth/logoutAction', async ({ route try { const provider = await oauthProviderFactory.getProvider() if (provider) { + // For OIDC providers, use signoutRedirect to end Keycloak session + if (provider.getUserManager) { + const userManager = provider.getUserManager() + if (userManager) { + await provider.clearAuthData() + + // Clear legacy auth tokens before redirect + localStorage.removeItem('accessToken') + localStorage.removeItem('authParams') + localStorage.removeItem('expiredIn') + localStorage.removeItem('isIdle') + localStorage.removeItem('version') + + dispatch(clearIntervalId()) + dispatch(setAuthToken('')) + + // Redirect to IdP logout endpoint — this will navigate away from the app + await userManager.signoutRedirect() + Review Comment: In the OIDC logout path, `provider.clearAuthData()` removes the `oidc-client-ts` user from storage before calling `userManager.signoutRedirect()`. `signoutRedirect()` typically relies on the stored user (e.g., `id_token` / `id_token_hint`) to perform a proper RP-initiated logout; clearing first can result in the IdP session not being terminated. Call `signoutRedirect()` before clearing the OIDC user (or explicitly pass the needed logout parameters), and then clear local app state (including `authUser`) afterward. ########## web-v2/web/src/lib/provider/session.js: ########## @@ -121,7 +122,20 @@ const AuthProvider = ({ children }) => { if (tokenToUse) { dispatch(setAuthToken(tokenToUse)) - user && dispatch(setAuthUser(user)) + + // Fetch server-resolved principal to ensure UI identity matches server-side + // identity derived from principalFields + principalMapper config + let authUser = user + try { + const [meErr, meRes] = await to(getAuthMeApi()) + if (!meErr && meRes && meRes.principal) { + authUser = { ...user, name: meRes.principal } + } + } catch (e) { + // Fallback to OIDC profile if /api/auth/me is unavailable Review Comment: The fallback comment mentions `/api/auth/me`, but the new endpoint and client call are `/api/authn/me`. Keeping this comment accurate matters for debugging and future maintenance. -- 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]
