Copilot commented on code in PR #11142:
URL: https://github.com/apache/gravitino/pull/11142#discussion_r3263516267
##########
web-v2/web/src/lib/auth/providers/oidc.js:
##########
@@ -20,6 +20,31 @@
import { BaseOAuthProvider } from './base'
import { UserManager, WebStorageStateStore } from 'oidc-client-ts'
+function getOidcAppBasePath() {
+ const basePath = process.env.NEXT_PUBLIC_BASE_PATH || ''
+
+ if (!basePath || basePath === '/') {
+ return ''
+ }
+
+ return basePath.replace(/\/$/, '')
+}
+
+function buildOidcUrl(pathname) {
+ return `${window.location.origin}${getOidcAppBasePath()}${pathname}`
+}
+
+function isOidcSecureOrigin() {
+ if (typeof window === 'undefined') {
+ return true
+ }
+
+ const { protocol, hostname } = window.location
Review Comment:
`isOidcSecureOrigin()` relies on `window.location.protocol` and
`window.location.hostname`. In this repo’s Vitest setup (`src/test/setup.js`),
`window.location` is mocked with only `href/origin/pathname/search/hash`, so
`protocol/hostname` are `undefined` and `initialize()` will always throw the
insecure-origin error (breaking tests and any similar mocks). Consider deriving
protocol/hostname from `window.location.origin` (e.g., via `new
URL(window.location.origin)`), or falling back to parsing `href/origin` when
`protocol`/`hostname` are missing.
##########
web-v2/web/src/lib/auth/providers/oidc.test.js:
##########
@@ -30,11 +30,67 @@ vi.mock('oidc-client-ts', () => ({
describe('OidcOAuthProvider', () => {
let provider
let mockUserManager
+ let originalBasePath
+ let originalLocation
+ let originalLocationDescriptor
+
+ const DEFAULT_SECURE_LOCATION = {
+ origin: 'https://localhost:3000',
+ protocol: 'https:',
+ hostname: 'localhost'
+ }
+
+ const restoreWindowLocation = () => {
+ if (originalLocationDescriptor) {
+ Object.defineProperty(window, 'location', originalLocationDescriptor)
+ } else {
+ Object.defineProperty(window, 'location', {
+ configurable: true,
+ value: originalLocation
+ })
+ }
+ }
+
+ const setWindowLocation = ({ origin, protocol, hostname }) => {
+ const mockedLocation = Object.create(originalLocation)
+
+ Object.defineProperties(mockedLocation, {
+ origin: {
+ configurable: true,
+ enumerable: true,
+ value: origin
+ },
+ protocol: {
+ configurable: true,
+ enumerable: true,
+ value: protocol
+ },
+ hostname: {
+ configurable: true,
+ enumerable: true,
+ value: hostname
+ }
+ })
+
+ Object.defineProperty(window, 'location', {
+ configurable: true,
+ value: mockedLocation
+ })
Review Comment:
`setWindowLocation()` redefines `window.location` with `configurable: true`.
However, the test harness defines `window.location` via `Object.defineProperty`
without `configurable: true` (`src/test/setup.js`), so the property is
non-configurable and this `defineProperty` call will throw a `TypeError`. To
keep tests stable, avoid changing the descriptor’s `configurable` flag
(preserve the existing descriptor), or update the location mock by assignment
when `writable: true` rather than redefining the property.
--
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]
