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

Address OPARequest #148

Closed
pranavr12 opened this issue Aug 19, 2024 · 7 comments
Closed

Address OPARequest #148

pranavr12 opened this issue Aug 19, 2024 · 7 comments

Comments

@pranavr12
Copy link
Contributor

pranavr12 commented Aug 19, 2024

Currently, in OpaS3SecurityFacadeProvider, the function, does two operations:

  1. Builds an OPA request - Builds a request containing the OPA document and the headers for the OPA request
  2. Sends the request to OPA server
    The return type of the function is SecurityResponse.

The issue we are currently facing is that while building the OPA request, based on some rules/ conditions we want to shortcut the OPA decision to either SecurityResponse - SUCCESS or FAILURE without sending a request to OPA server.
We could have the opaS3SecurityMapper.toRequest returning a wrapper containing (OpaRequest | SecurityResponse), and based on SecurityResponse we shortcut the decision or proceed with further steps. WDYT ?
How should we handle this ?

@Randgalt , @vagaerg , @mosiac1

@Randgalt
Copy link
Member

You'd only short circuit on FAILURE or might it be both SUCCESS and FAILURE? If the short circuit is just FAILURE is this an exceptional (rare) case? If so, I'd have opaS3SecurityMapper throw an exception indicating failure.

@pranavr12
Copy link
Contributor Author

@Randgalt We would want to shortcut it mostly on FAILURE. The failure scenario could be fairly common - In our case, we call an external service to fetch the table name. This external service can send a 404, error response or it could be unavailable.

@Randgalt
Copy link
Member

I suggest throwing an exception or making a new response as you offered: (OpaRequest | Optional<SecurityResponse>)

@mosiac1
Copy link
Contributor

mosiac1 commented Aug 21, 2024

I prefer creating a new response type, I like the no-throw contract we have for the security classes, its easier to track where decisions are made.

I was thinking of OpaRequest | SecurityResponse, which we can now do with sealed interfaces.

In (OpaRequest | Optional<SecurityResponse>), what would an empty Optional represent?

@Randgalt
Copy link
Member

I'm assuming the SecurityResponse is optional.

@pranavr12
Copy link
Contributor Author

As @mosiac1 , we can do this by using sealed interfaces where the types are OpaRequest and SecurityResponse. I can raise a draft PR for this

@pranavr12
Copy link
Contributor Author

Closed by #150

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants