-
Notifications
You must be signed in to change notification settings - Fork 2
Add group configuration and auth validation #307
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 |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ import {describe, expect, it} from 'vitest'; | |
| import {Secret} from '@databricks/sdk-core/profiles/browser'; | ||
| import type {Profile} from '@databricks/sdk-core/profiles/browser'; | ||
|
|
||
| import type {Header} from '../../../src/auth'; | ||
| import type {Credentials, Header} from '../../../src/auth'; | ||
| import { | ||
| DefaultCredentials, | ||
| m2mStrategy, | ||
|
|
@@ -18,19 +18,38 @@ import type {DefaultCredentialsErrorCode} from '../../../src/credentials/default | |
|
|
||
| const HOST = 'https://workspace.example'; | ||
|
|
||
| function configuredStrategy(label: string): Strategy { | ||
| function configuredStrategy( | ||
| label: string, | ||
| supportsGroupAssumption = true, | ||
| onConfigure?: (profile: Profile) => void | ||
| ): Strategy { | ||
| return { | ||
| name: label, | ||
| configure: () => ({ | ||
| name: () => label, | ||
| authHeaders: () => | ||
| Promise.resolve([{key: 'X-Test-Strategy', value: label}]), | ||
| }), | ||
| supportsGroupAssumption, | ||
| configure: (profile): Credentials => { | ||
| onConfigure?.(profile); | ||
| return { | ||
| name: () => label, | ||
| authHeaders: () => | ||
| Promise.resolve([{key: 'X-Test-Strategy', value: label}]), | ||
| }; | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| function unconfiguredStrategy(label: string): Strategy { | ||
| return {name: label, configure: () => undefined}; | ||
| function unconfiguredStrategy( | ||
| label: string, | ||
| supportsGroupAssumption = true, | ||
| onConfigure?: () => void | ||
| ): Strategy { | ||
| return { | ||
| name: label, | ||
| supportsGroupAssumption, | ||
| configure: (): undefined => { | ||
| onConfigure?.(); | ||
| return undefined; | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| const loaderFor = | ||
|
|
@@ -87,6 +106,7 @@ describe('DefaultCredentials chain', () => { | |
| let buildCount = 0; | ||
| const strategy: Strategy = { | ||
| name: 'counting', | ||
| supportsGroupAssumption: true, | ||
| configure: () => { | ||
| buildCount += 1; | ||
| return { | ||
|
|
@@ -101,6 +121,35 @@ describe('DefaultCredentials chain', () => { | |
| expect(buildCount).toBe(1); | ||
| }); | ||
|
|
||
| it('skips unsupported strategies when a group is configured', async () => { | ||
| let unsupportedCalls = 0; | ||
| const creds = new DefaultCredentials( | ||
| [ | ||
| configuredStrategy('pat', false, () => { | ||
| unsupportedCalls += 1; | ||
| }), | ||
| configuredStrategy('oauth-m2m'), | ||
| ], | ||
| loaderFor({host: HOST, groupId: 'group-123'}) | ||
| ); | ||
|
|
||
| expect(await creds.authHeaders()).toEqual([ | ||
| {key: 'X-Test-Strategy', value: 'oauth-m2m'}, | ||
| ]); | ||
| expect(unsupportedCalls).toBe(0); | ||
| }); | ||
|
|
||
| it('preserves normal strategy ordering when the group is empty', async () => { | ||
| const creds = new DefaultCredentials( | ||
| [configuredStrategy('pat', false), configuredStrategy('oauth-m2m')], | ||
| loaderFor({host: HOST, groupId: ''}) | ||
| ); | ||
|
|
||
| expect(await creds.authHeaders()).toEqual([ | ||
| {key: 'X-Test-Strategy', value: 'pat'}, | ||
| ]); | ||
| }); | ||
|
|
||
| it('invokes the profile loader exactly once', async () => { | ||
| let loaderCalls = 0; | ||
| const loader = (): Promise<Profile> => { | ||
|
|
@@ -113,6 +162,29 @@ describe('DefaultCredentials chain', () => { | |
| expect(loaderCalls).toBe(1); | ||
| }); | ||
|
|
||
| it('does not configure a fallback after the selected strategy fails', async () => { | ||
|
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. ditto |
||
| const selectedError = new Error('selected provider failed'); | ||
| let fallbackCalls = 0; | ||
| const selected: Strategy = { | ||
| name: 'oauth-m2m', | ||
| supportsGroupAssumption: true, | ||
| configure: () => ({ | ||
| name: () => 'oauth-m2m', | ||
| authHeaders: () => Promise.reject(selectedError), | ||
| }), | ||
| }; | ||
| const fallback = configuredStrategy('fallback', true, () => { | ||
| fallbackCalls += 1; | ||
| }); | ||
| const creds = new DefaultCredentials( | ||
| [selected, fallback], | ||
| loaderFor({host: HOST, groupId: 'group-123'}) | ||
| ); | ||
|
|
||
| await expect(creds.authHeaders()).rejects.toBe(selectedError); | ||
| expect(fallbackCalls).toBe(0); | ||
| }); | ||
|
|
||
| const errorCases: { | ||
| name: string; | ||
| strategies: readonly Strategy[]; | ||
|
|
@@ -141,6 +213,37 @@ describe('DefaultCredentials chain', () => { | |
| profile: {host: HOST, authType: 'pat'}, | ||
| wantCode: 'NO_AUTH_CONFIGURED', | ||
| }, | ||
| { | ||
| name: 'throws GROUP_ROLE_UNSUPPORTED for an explicitly selected PAT strategy', | ||
| strategies: [patStrategy, m2mStrategy], | ||
| profile: { | ||
| host: HOST, | ||
| token: new Secret('dapi-abc'), | ||
| groupId: 'group-123', | ||
| authType: 'pat', | ||
| }, | ||
| wantCode: 'GROUP_ROLE_UNSUPPORTED', | ||
| }, | ||
| { | ||
| name: 'throws GROUP_ROLE_UNSUPPORTED for an explicitly selected CLI strategy', | ||
| strategies: [configuredStrategy('databricks-cli', false)], | ||
| profile: { | ||
| host: HOST, | ||
| groupId: 'group-123', | ||
| authType: 'databricks-cli', | ||
| }, | ||
| wantCode: 'GROUP_ROLE_UNSUPPORTED', | ||
| }, | ||
| { | ||
| name: 'throws NO_AUTH_CONFIGURED when grouped strategies are exhausted', | ||
| strategies: [ | ||
| configuredStrategy('pat', false), | ||
| unconfiguredStrategy('oauth-m2m'), | ||
| configuredStrategy('databricks-cli', false), | ||
| ], | ||
| profile: {host: HOST, groupId: 'group-123'}, | ||
| wantCode: 'NO_AUTH_CONFIGURED', | ||
| }, | ||
| ]; | ||
|
|
||
| it.each(errorCases)('$name', async ({strategies, profile, wantCode}) => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,11 @@ import type {Stats} from 'node:fs'; | |
| import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; | ||
|
|
||
| import type {U2mCredentialsErrorCode} from '../../src/credentials'; | ||
| import {U2mCredentialsError, newU2mCredentials} from '../../src/credentials'; | ||
| import { | ||
| U2mCredentialsError, | ||
| defaultCredentials, | ||
| newU2mCredentials, | ||
| } from '../../src/credentials'; | ||
|
|
||
| type ExecFileCallback = ( | ||
| err: Error | null, | ||
|
|
@@ -83,6 +87,23 @@ describe('newU2mCredentials', () => { | |
| vi.unstubAllEnvs(); | ||
| }); | ||
|
|
||
| it('rejects grouped explicit CLI auth before invoking the CLI', async () => { | ||
|
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. ditto should be part of below table. |
||
| const credentials = defaultCredentials({ | ||
| profile: { | ||
| name: DEFAULT_PROFILE, | ||
| host: 'https://workspace.example', | ||
| authType: 'databricks-cli', | ||
| groupId: 'group-123', | ||
| }, | ||
| }); | ||
|
|
||
| await expect(credentials.authHeaders()).rejects.toMatchObject({ | ||
| code: 'GROUP_ROLE_UNSUPPORTED', | ||
| }); | ||
| expect(statMock).not.toHaveBeenCalled(); | ||
| expect(execFileMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| const successCases: { | ||
| name: string; | ||
| profile: string; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ function findDef(field: string): PropertyDef { | |
| } | ||
|
|
||
| const STRING_DEF = findDef('host'); | ||
| const GROUP_ID_DEF = findDef('groupId'); | ||
| const SECRET_DEF = findDef('token'); | ||
|
|
||
| describe('property set and get', () => { | ||
|
|
@@ -36,6 +37,12 @@ describe('property set and get', () => { | |
| raw: 'https://x.com?a=1&b=2', | ||
| wantGet: 'https://x.com?a=1&b=2', | ||
| }, | ||
| { | ||
| name: 'group ID', | ||
| def: GROUP_ID_DEF, | ||
| raw: 'group-123', | ||
| wantGet: 'group-123', | ||
| }, | ||
| // Secret properties. | ||
| { | ||
| name: 'secret: plain value', | ||
|
|
@@ -77,6 +84,13 @@ describe('property set and get', () => { | |
| }); | ||
|
|
||
| describe('PROPERTY_DEFS', () => { | ||
| it('maps groupId to the Databricks environment and INI names', () => { | ||
| expect(GROUP_ID_DEF).toMatchObject({ | ||
| envVar: 'DATABRICKS_GROUP_ID', | ||
| iniKey: 'group_id', | ||
| }); | ||
| }); | ||
|
Comment on lines
86
to
+92
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. ditto we should have existing table for this. |
||
|
|
||
| it('should cover every Profile field except name and extra', () => { | ||
| // Set every property to a sentinel value via PROPERTY_DEFS, then check | ||
| // that no Profile field was missed. The source of truth is the Profile | ||
|
|
||
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.
These tests should be part of the above table test.