trinodb / aws-proxy

Proxy for S3
Apache License 2.0
7 stars 3 forks source link

Bind Identity to S3SecurityController #145

Closed pranavr12 closed 1 month ago

vagaerg commented 1 month ago

LGTM.

I would to add at least one test that binds an CredentialsProvider that will return a non-empty identify and check that the identity reaches S3SecurityFacadeProvider#securityFacadeForRequest. We should also bind a custom Identity class. I don't think we currently have any tests that combine all these functionalities.

Updating TestS3SecurityController with this might make sense. @vagaerg WDYT?

Yup that makes sense. (Edit): Also, my initial reaction to this was that we can rely on the tests of plugins like the OPA security interface (admittedly, once we create some more thorough tests) to indirectly assert that the core is passing down identities properly. But perhaps it makes sense to bring these tests into the core now

I was also going to suggest waiting for @Randgalt to have a look given this is an SPI change

pranavr12 commented 1 month ago

Currently, the TestingCredentials - https://github.com/trinodb/aws-proxy/blob/main/trino-aws-proxy/src/test/java/io/trino/aws/proxy/server/testing/TestingUtil.java#L57 is used by the s3 client. should we add Identity to this ? If not, how should we plan to test this or should we rely on other tests that the identity from credentials provider is passed to OPA correctly? Another workaround - In the test class, we could modify the stored credentials in the TestingCredentialsProvider to fetch and restore new credential with identity

Added the Identoty to TestingCredentials. I have also added an assert in the TestOpaSecurity class to verify the identity