feat: Grant access to schema for role (#36)

[stephanpelikan]

Fixes #36

Tests are missing since I don't know how to test this, write them and unfortunately my time is limited. I've tested the code for creating and deleting a schema manually in my project successfully.

[berenddeboer]

Yeah, this needs to come with a test. Just look at the handler.test.ts. Not merging code without an actual test that runs that code. And yes, everyone's time is limited.

[berenddeboer]

Yeah, that needs doing (and be tested).

[stephanpelikan]

You did those notes as well:

https://github.com/berenddeboer/cdk-rds-sql/blob/main/src/handler.ts#L118
https://github.com/berenddeboer/cdk-rds-sql/blob/main/src/handler.ts#L160

If the handler would be prepared to get old props as well I would have done this check. It is out of the scope of this little change to refactor the handler.

[berenddeboer]

You're repeating yourself a lot, so please put the grant + revoke schema into their own function, so you can call that instead of copying the same code.

[stephanpelikan]

You're doing the same in may other places:

https://github.com/berenddeboer/cdk-rds-sql/blob/main/src/handler.ts#L100
https://github.com/berenddeboer/cdk-rds-sql/blob/main/src/handler.ts#L145

and

https://github.com/berenddeboer/cdk-rds-sql/blob/main/src/handler.ts#L187
https://github.com/berenddeboer/cdk-rds-sql/blob/main/src/handler.ts#L211

and

https://github.com/berenddeboer/cdk-rds-sql/blob/main/src/handler.ts#L211
https://github.com/berenddeboer/cdk-rds-sql/blob/main/src/handler.ts#L140

However, I moved the statements into functions.

[stephanpelikan]

The tests you did are quite easy to achieve. It is hard to figure out about those grants. I still searching for those queries which can be used for tests...

[stephanpelikan]

I found a way to test for those grants on stackoverflow. I added the test and I hope it is sufficient.

[berenddeboer]

I have another idea: should we introduce a concept like schema access or so? That can do what your role property does. Would make it easier to grant access to the schema to more roles, as this just handles only the one role case.

What do you think?

Will have a look at your changes later.

[stephanpelikan]

Well yes. But this also applies to databases. I use one RDS serverless instance for all app in my Kubernetes cluster. Within that instance I create a database per application. I only use the public schema, but roles created for and applied to those databases are not allowed to use public. Actually, whatever is done needs to be applies to databases and schemas. One uses multiple databases and another multiple schemas and others multiple databases and multiple schemas.

What I often do is: Keep it simple for the simple case and provide a more flexible way for more complex scenarios. For that simple scenario I would change the role properties of database and schema to roles: Role | Role[]. But if you provide a separate construct for granting and it's that easy for the simple cases then it would be fine as well. Maybe something like this:

interface RoleGrantProps {
  provider: Provider
  role: Role | Role[]
  target: Database | Schema
  grants: 'all' | 'readonly' | string | string[]
}

For the custom grants I don't know enough about what is typically needed. Maybe it is the best the simply allow grant-SQLs here.

Hower, I build CDK stacks for a customer as a draft and would need something released. If you don't want to release the current version in preference of a separate construct, then please let me know so I can replace the new role property by a SQL construct.

[berenddeboer]

Merged, thanks for all the work @stephanpelikan. I agree, we can improve things later.