Skip to content
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

fix(redshift): UserTablePriviliges to track changes using table IDs #26955

Merged
merged 34 commits into from
Oct 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
3546c5d
refactor to use table id over table name
Rizxcviii Mar 30, 2023
88582a0
post integration snapshot
Rizxcviii Mar 30, 2023
235739f
Merge branch 'main' into refactor/UserTablePriviligies
Rizxcviii Mar 31, 2023
ee55f00
Merge branch 'main' into refactor/UserTablePriviligies
Rizxcviii Apr 14, 2023
8b41c77
post merge fix
Rizxcviii Apr 14, 2023
b809529
Merge branch 'main' into refactor/UserTablePriviligies
Rizxcviii Apr 17, 2023
e4841f2
change to test file, was failing build
Rizxcviii Apr 17, 2023
5c585d7
integ test
Rizxcviii Apr 17, 2023
8ff3f62
Merge branch 'main' into refactor/UserTablePriviligies
Rizxcviii Apr 26, 2023
ff98842
post merge
Rizxcviii Apr 26, 2023
3f16d35
issue was name of file
Rizxcviii Apr 26, 2023
e81d828
revert
Rizxcviii Apr 26, 2023
93c50ee
Merge branch 'main' into refactor/UserTablePriviligies
corymhall May 5, 2023
da13a0b
Merge branch 'main' into refactor/UserTablePriviligies
Rizxcviii May 23, 2023
6f18b15
Merge branch 'main' into refactor/UserTablePriviligies
Rizxcviii Jul 28, 2023
a783f38
removing unecessary table id, used node.id instead
Rizxcviii Jul 28, 2023
97d2467
adding back in privileges.ts
Rizxcviii Jul 31, 2023
8986bb8
reversing unecessary changes
Rizxcviii Jul 31, 2023
f4fcb15
removing id
Rizxcviii Jul 31, 2023
d9283ef
integ test
Rizxcviii Jul 31, 2023
61ae00b
reverting machine-image
Rizxcviii Aug 2, 2023
fcd2be4
when table id is removed, that means table is deleted
Rizxcviii Aug 2, 2023
bd8a062
bump
Rizxcviii Aug 23, 2023
d9c0b2b
Merge branch 'main' into refactor/UserTablePriviligies
Rizxcviii Aug 24, 2023
4c8d9b0
Merge branch 'main' into refactor/UserTablePriviligies
Rizxcviii Sep 1, 2023
0b4b7ba
Merge branch 'aws:main' into refactor/UserTablePriviligies
Rizxcviii Sep 4, 2023
ab35e58
pkglint
Rizxcviii Sep 4, 2023
ceb5513
Merge branch 'main' into refactor/UserTablePriviligies
Rizxcviii Sep 5, 2023
5c0b036
reverting pkglint
Rizxcviii Sep 5, 2023
dc31303
reverting pkglint
Rizxcviii Sep 5, 2023
7f0132f
Merge branch 'main' into refactor/UserTablePriviligies
Rizxcviii Sep 11, 2023
3ea0701
integ test
Rizxcviii Sep 11, 2023
b00614e
adding inline comments
Rizxcviii Sep 25, 2023
75ab7a1
Merge branch 'main' into refactor/UserTablePriviligies
mergify[bot] Oct 6, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
/* eslint-disable-next-line import/no-unresolved */
import * as AWSLambda from 'aws-lambda';
import { TablePrivilege, UserTablePrivilegesHandlerProps } from '../handler-props';
import { executeStatement } from './redshift-data';
import { ClusterProps } from './types';
import { makePhysicalId } from './util';
import { TablePrivilege, UserTablePrivilegesHandlerProps } from '../handler-props';

export async function handler(props: UserTablePrivilegesHandlerProps & ClusterProps, event: AWSLambda.CloudFormationCustomResourceEvent) {
const username = props.username;
Expand Down Expand Up @@ -62,10 +62,26 @@ async function updatePrivileges(
}

const oldTablePrivileges = oldResourceProperties.tablePrivileges;
if (oldTablePrivileges !== tablePrivileges) {
await revokePrivileges(username, oldTablePrivileges, clusterProps);
await grantPrivileges(username, tablePrivileges, clusterProps);
return { replace: false };
const tablesToRevoke = oldTablePrivileges.filter(({ tableId, actions }) => (
tablePrivileges.find(({ tableId: otherTableId, actions: otherActions }) => (
tableId === otherTableId && actions.some(action => !otherActions.includes(action))
))
));
if (tablesToRevoke.length > 0) {
await revokePrivileges(username, tablesToRevoke, clusterProps);
}

const tablesToGrant = tablePrivileges.filter(({ tableId, tableName, actions }) => {
const tableAdded = !oldTablePrivileges.find(({ tableId: otherTableId, tableName: otherTableName }) => (
tableId === otherTableId && tableName === otherTableName
));
const actionsAdded = oldTablePrivileges.find(({ tableId: otherTableId, actions: otherActions }) => (
tableId === otherTableId && otherActions.some(action => !actions.includes(action))
));
return tableAdded || actionsAdded;
});
if (tablesToGrant.length > 0) {
await grantPrivileges(username, tablesToGrant, clusterProps);
}

return { replace: false };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export interface TableHandlerProps {
}

export interface TablePrivilege {
readonly tableId: string;
readonly tableName: string;
readonly actions: string[];
}
Expand Down
31 changes: 19 additions & 12 deletions packages/@aws-cdk/aws-redshift-alpha/lib/private/privileges.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import * as cdk from 'aws-cdk-lib/core';
import { Construct } from 'constructs';
import { DatabaseQuery } from './database-query';
import { HandlerName } from './database-query-provider/handler-name';
import { TablePrivilege as SerializedTablePrivilege, UserTablePrivilegesHandlerProps } from './handler-props';
import { DatabaseOptions } from '../database-options';
import { ITable, TableAction } from '../table';
import { IUser } from '../user';
import { DatabaseQuery } from './database-query';
import { HandlerName } from './database-query-provider/handler-name';
import { TablePrivilege as SerializedTablePrivilege, UserTablePrivilegesHandlerProps } from './handler-props';

/**
* The Redshift table and action that make up a privilege that can be granted to a Redshift user.
Expand Down Expand Up @@ -63,23 +63,30 @@ export class UserTablePrivileges extends Construct {
tablePrivileges: cdk.Lazy.any({
produce: () => {
const reducedPrivileges = this.privileges.reduce((privileges, { table, actions }) => {
const tableName = table.tableName;
if (!(tableName in privileges)) {
privileges[tableName] = [];
const tableId = table.node.id;
if (!(tableId in privileges)) {
privileges[tableId] = {
tableName: table.tableName,
actions: [],
};
}
actions = actions.concat(privileges[tableName]);
actions = actions.concat(privileges[tableId].actions);
if (actions.includes(TableAction.ALL)) {
actions = [TableAction.ALL];
}
if (actions.includes(TableAction.UPDATE) || actions.includes(TableAction.DELETE)) {
actions.push(TableAction.SELECT);
}
privileges[tableName] = Array.from(new Set(actions));
privileges[tableId] = {
tableName: table.tableName,
actions: Array.from(new Set(actions)),
};
return privileges;
}, {} as { [key: string]: TableAction[] });
const serializedPrivileges: SerializedTablePrivilege[] = Object.entries(reducedPrivileges).map(([tableName, actions]) => ({
tableName: tableName,
actions: actions.map(action => TableAction[action]),
}, {} as { [key: string]: { tableName: string, actions: TableAction[] } });
const serializedPrivileges: SerializedTablePrivilege[] = Object.entries(reducedPrivileges).map(([tableId, config]) => ({
tableId,
tableName: config.tableName,
actions: config.actions.map(action => TableAction[action]),
}));
return serializedPrivileges;
},
Expand Down
Rizxcviii marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ import type * as AWSLambda from 'aws-lambda';

const username = 'username';
const tableName = 'tableName';
const tablePrivileges = [{ tableName, actions: ['INSERT', 'SELECT'] }];
const tableId = 'tableId';
const actions = ['INSERT', 'SELECT'];
const tablePrivileges = [{ tableId, tableName, actions }];
const clusterName = 'clusterName';
const adminUserArn = 'adminUserArn';
const databaseName = 'databaseName';
Expand Down Expand Up @@ -147,22 +149,99 @@ describe('update', () => {
}));
});

test('does not replace when privileges change', async () => {
test('does not replace when table name is changed', async () => {
const newTableName = 'newTableName';
const newTablePrivileges = [{ tableName: newTableName, actions: ['DROP'] }];
const newTablePrivileges = [{ tableId, tableName: newTableName, actions }];
const newResourceProperties = {
...resourceProperties,
tablePrivileges: newTablePrivileges,
};

// Checking if the table resource has not been recreated
await expect(managePrivileges(newResourceProperties, event)).resolves.toMatchObject({
PhysicalResourceId: physicalResourceId,
});
// Upon a table name change, Redshift maintains the same table priviliges as before.
// The name of the table has changed, a new table has not been created.
// Therefore 'REVOKE' statements should not be used.
expect(mockExecuteStatement).not.toHaveBeenCalledWith(expect.objectContaining({
Sql: `REVOKE INSERT, SELECT ON ${newTableName} FROM ${username}`,
}));
// Likewise, here the table name has changed, so the current priviliges will still be intact.
expect(mockExecuteStatement).not.toHaveBeenCalledWith(expect.objectContaining({
Sql: expect.stringMatching(new RegExp(`.+ ON ${tableName} TO ${username}`)),
}));
});

test('does not replace when table actions are changed', async () => {
const newTablePrivileges = [{ tableId, tableName, actions: ['DROP'] }];
const newResourceProperties = {
...resourceProperties,
tablePrivileges: newTablePrivileges,
};

// Checking if the table resource has not been recreated
await expect(managePrivileges(newResourceProperties, event)).resolves.toMatchObject({
PhysicalResourceId: physicalResourceId,
});
// Old actions are REVOKED, as they are not included in the list
expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
Sql: `REVOKE INSERT, SELECT ON ${tableName} FROM ${username}`,
}));
// New actions are GRANTED, as they are included in the list
expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
Sql: `GRANT DROP ON ${newTableName} TO ${username}`,
Sql: `GRANT DROP ON ${tableName} TO ${username}`,
}));
});
});

test('does not replace when table id is changed', async () => {
const newTableId = 'newTableId';
const newTablePrivileges = [{ tableId: newTableId, tableName, actions }];
const newResourceProperties = {
...resourceProperties,
tablePrivileges: newTablePrivileges,
};

// Checking if the table resource has not been recreated, we are not changing on table id either.
// Due to the construct only needing to be changed on a new user, not a new table
await expect(managePrivileges(newResourceProperties, event)).resolves.toMatchObject({
PhysicalResourceId: physicalResourceId,
});
// Upon removal of the old table, the priviliges will also be revoked automatically,
// as the table no longer exists.
// Calling REVOKE statments on a non-existing table will throw errors by Redshift
expect(mockExecuteStatement).not.toHaveBeenCalledWith(expect.objectContaining({
Sql: expect.stringMatching(new RegExp(`REVOKE .+ ON ${tableName} FROM ${username}`)),
}));
// Adds the permissions onto the newly created table
expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
Sql: expect.stringMatching(new RegExp(`GRANT .+ ON ${tableName} TO ${username}`)),
}));
});

test('does not replace when table id is appended', async () => {
const newTablePrivileges = [{ tableId: 'newTableId', tableName, actions }];
const newResourceProperties = {
...resourceProperties,
tablePrivileges: newTablePrivileges,
};

const newEvent = {
...event,
OldResourceProperties: {
...event.OldResourceProperties,
tablePrivileges: [{ tableName, actions }],
},
};

// Checking if the table resource has not been recreated
await expect(managePrivileges(newResourceProperties, newEvent)).resolves.toMatchObject({
PhysicalResourceId: physicalResourceId,
});
// Upon initial deployment from non table id usage to table id usage,
// permissions would not need to be granted/revoked, as the table should already exist
expect(mockExecuteStatement).not.toHaveBeenCalledWith(expect.objectContaining({
Sql: expect.stringMatching(new RegExp(`.+ ON ${tableName} FROM ${username}`)),
}));
});
});
Loading