-
Notifications
You must be signed in to change notification settings - Fork 26
Support X-Trino-Role header
#322
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 |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ import ( | |
| const ( | ||
| accessTokenKey = "accessToken" | ||
| trinoUserHeader = "X-Trino-User" | ||
| trinoRoleHeader = "X-Trino-Role" | ||
|
Author
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. @nineinchnick @ksobolew @lukasz-walkiewicz @augurpl @kokosing @mwd410 please review |
||
| trinoClientTagsKey = "X-Trino-Client-Tags" | ||
| bearerPrefix = "Bearer " | ||
| ) | ||
|
|
@@ -41,6 +42,10 @@ func (ds *SQLDatasourceWithTrinoUserContext) QueryData(ctx context.Context, req | |
| ctx = context.WithValue(ctx, trinoUserHeader, user) | ||
| } | ||
|
|
||
| if settings.Role != "" { | ||
| ctx = context.WithValue(ctx, trinoRoleHeader, settings.Role) | ||
| } | ||
|
|
||
| if settings.ClientTags != "" { | ||
| ctx = context.WithValue(ctx, trinoClientTagsKey, settings.ClientTags) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,9 @@ export class ConfigEditor extends PureComponent<Props, State> { | |
| const onImpersonationUserChange = (event: ChangeEvent<HTMLInputElement>) => { | ||
| onOptionsChange({...options, jsonData: {...options.jsonData, impersonationUser: event.target.value}}) | ||
| }; | ||
| const onRoleChange = (event: ChangeEvent<HTMLInputElement>) => { | ||
| onOptionsChange({...options, jsonData: {...options.jsonData, role: event.target.value}}) | ||
| }; | ||
| const onClientTagsChange = (event: ChangeEvent<HTMLInputElement>) => { | ||
| onOptionsChange({...options, jsonData: {...options.jsonData, clientTags: event.target.value}}) | ||
| }; | ||
|
|
@@ -75,6 +78,19 @@ export class ConfigEditor extends PureComponent<Props, State> { | |
| /> | ||
| </InlineField> | ||
| </div> | ||
| <div className="gf-form-inline"> | ||
| <InlineField | ||
| label="Role" | ||
| tooltip="For X-Trino-Role" | ||
|
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. I'm not sure that tooltip is very helpful :) |
||
| labelWidth={26} | ||
| > | ||
| <Input | ||
| value={options.jsonData?.role ?? ''} | ||
| onChange={onRoleChange} | ||
| width={40} | ||
| /> | ||
| </InlineField> | ||
| </div> | ||
| <div className="gf-form-inline"> | ||
| <InlineField | ||
| label="Client Tags" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,12 @@ async function setupDataSourceWithClientTags(page: Page, clientTags: string) { | |
| await page.getByTestId('data-testid Data source settings page Save and Test button').click(); | ||
| } | ||
|
|
||
| async function setupDataSourceWithRole(page: Page, role: string) { | ||
| await page.getByTestId('data-testid Datasource HTTP settings url').fill('http://trino:8080'); | ||
| await page.locator('div').filter({hasText: /^Role$/}).locator('input').fill(role); | ||
| await page.getByTestId('data-testid Data source settings page Save and Test button').click(); | ||
| } | ||
|
|
||
| async function runQueryAndCheckResults(page: Page) { | ||
| await page.getByLabel(EXPORT_DATA).click(); | ||
| await page.getByTestId('data-testid TimePicker Open Button').click(); | ||
|
|
@@ -91,3 +97,34 @@ test('test with client tags', async ({ page }) => { | |
| await setupDataSourceWithClientTags(page, 'tag1,tag2,tag3'); | ||
| await runQueryAndCheckResults(page); | ||
| }); | ||
|
|
||
| test('test with role', async ({ page }) => { | ||
| await login(page); | ||
| await goToTrinoSettings(page); | ||
| await setupDataSourceWithRole(page, 'hive=ROLE{admin}'); | ||
|
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. Oh, so the user needs to know the proper format of this header's value. I'm not sure this is a good idea. I never remember that :) But I'm not familiar with this repository - is this something that only very technical people will use?
Author
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. I had the same reasoning initially #322 (comment) 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. You could make it very complicated with a checkbox for "system or catalog role", then if it's a catalog role then an input box appears to enter the catalog name :) 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. Also, I guess there should be a test that sets a system role too
Author
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. It's not possible to test successful path scenario on trino yet. |
||
| await runRoleQuery(page); | ||
| await expect(page.getByTestId('data-testid table body')).toContainText(/.*admin.*/); | ||
|
|
||
| }); | ||
|
|
||
| test('test without role', async ({ page }) => { | ||
| await login(page); | ||
| await goToTrinoSettings(page); | ||
| await setupDataSourceWithRole(page, ''); | ||
| await runRoleQuery(page); | ||
| await expect(page.getByText(/Access Denied: Cannot show roles/)).toBeVisible(); | ||
| }); | ||
|
|
||
| async function runRoleQuery(page: Page) { | ||
| await page.getByLabel(EXPORT_DATA).click(); | ||
| await page.locator('div').filter({hasText: /^Format asChoose$/}).locator('svg').click(); | ||
| await page.getByRole('option', {name: 'Table'}).click(); | ||
| await setQuery(page, 'SHOW ROLES FROM hive') | ||
| await page.getByTestId('data-testid Code editor container').click(); | ||
| await page.getByTestId('data-testid RefreshPicker run button').click(); | ||
| } | ||
|
|
||
| async function setQuery(page: Page, query: string) { | ||
| await page.getByTestId('data-testid Code editor container').click({ clickCount: 4 }); | ||
| await page.keyboard.type(query); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| connector.name=hive | ||
| hive.metastore=file | ||
| hive.metastore.catalog.dir=/tmp/metastore | ||
| hive.security=sql-standard | ||
| fs.hadoop.enabled=true |
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.
Is there a timeout?
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.
There should be job default timeout.
Also this code is an adapted version of the preexisting behaviour for Keycloak.