Skip to content

feat!: Add authenticationResponse context to OpaInput#85

Merged
fhennig merged 2 commits intostackabletech:mainfrom
jakubmatyszewski:context
Mar 19, 2024
Merged

feat!: Add authenticationResponse context to OpaInput#85
fhennig merged 2 commits intostackabletech:mainfrom
jakubmatyszewski:context

Conversation

@jakubmatyszewski
Copy link
Copy Markdown
Contributor

@jakubmatyszewski jakubmatyszewski commented Mar 15, 2024

Description

Adding AuthenticationResponse context to the OpaInput. This can become useful when paired with authentication done through druid-pac4j extension - eg. access to Okta user profile details (groups).

This PR can give potential context: apache/druid#16109

@stackable-bot
Copy link
Copy Markdown
Contributor

stackable-bot commented Mar 15, 2024

CLA assistant check
All committers have signed the CLA.

@lfrancke
Copy link
Copy Markdown
Member

Thank you for the contribution.
We'll take a look next week. Have a great weekend.

@fhennig
Copy link
Copy Markdown
Contributor

fhennig commented Mar 18, 2024

Hi! Thanks for adding this, I had a look, it seems to me like a good idea to add this information to the OpaInput.

The information in the OpaInput was just a first try at implementing OPA authorization for Druid, now, looking at Trino and Kafka I think it makes more sense to provide as much information as possible to OPA. Because of that, I think it would be best to put identity and context both in an authenticationResult key, to reflect how Druid does it internally. This way it's also possible to add the other two fields (authorizerName and authenticatedBy) in there.

Could I ask you to make this change? So instead of

    {
        user: <String: user name>
        action: <String: READ|WRITE>
        resource: {
            name: <String>
            type: <String>
        }
        context: Map<String, Object>
    }

like this:

    {
        authenticationResult: {
            identity: <String: user name>,
            context: Map<String, Object>
        },
        action: <String: READ|WRITE>
        resource: {
            name: <String>
            type: <String>
        }
    }

@jakubmatyszewski
Copy link
Copy Markdown
Contributor Author

@fhennig, I've added authenticationResult to the input. Let me know if that's alright

Copy link
Copy Markdown
Contributor

@fhennig fhennig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, looks great!

I'll see how we can release this. (Also the CI job didn't work because of external contribution, I'll also look into getting that fixed)

@fhennig fhennig enabled auto-merge March 19, 2024 08:50
@lfrancke
Copy link
Copy Markdown
Member

We (hopefully) fixed a bug in our Github action. Would you mind upadting your PR to the latest changes from main?

@fhennig fhennig added this pull request to the merge queue Mar 19, 2024
Merged via the queue into stackabletech:main with commit 0142a7e Mar 19, 2024
@jakubmatyszewski jakubmatyszewski deleted the context branch March 19, 2024 09:37
@fhennig
Copy link
Copy Markdown
Contributor

fhennig commented Mar 19, 2024

thanks for your contribution! We are tracking the release of this into the operator in this ticket: stackabletech/druid-operator#533

@sbernauer sbernauer changed the title Add authenticationResponse context to OpaInput feat!: Add authenticationResponse context to OpaInput Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants