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

Make OAuth scope optional #30

Merged
merged 3 commits into from
Feb 20, 2020
Merged

Conversation

klalafaryan
Copy link
Member

Some OAuth Authorization servers such as AWS Cognito don't provide the scope in the response. For example:

{
    "access_token": "{JWT_TOKEN}",
    "expires_in": 3600,
    "token_type": "Bearer"
}

This PR makes scope property optional which gives us possibility to integrate with AWS Cognito. (Authorization servers which are not providing scope in the response)

Signed-off-by: klalafaryan <konstantin.lalafaryan@blacklane.com>
Signed-off-by: klalafaryan <konstantin.lalafaryan@blacklane.com>
Signed-off-by: klalafaryan <konstantin.lalafaryan@blacklane.com>
@mstruk
Copy link
Contributor

mstruk commented Feb 19, 2020

The fix is in fact on the Kafka client (the client side of OAuth) - it doesn't really concern the Kafka Broker unless inter-broker communication goes over OAuth protected listener (which in Strimzi Kafka Operator it doesn't).

I think adding the comment in the code is unnecessary, but overall the PR looks good.

@klalafaryan Thank you for your contribution.

@mstruk mstruk added this to the 0.4.0 milestone Feb 19, 2020
Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

LGTM but how are we going to test this?

@mstruk
Copy link
Contributor

mstruk commented Feb 20, 2020

@ppatierno I guess we won't test it immediately - the addressed situation never occurs in our current test setups (using Keycloak and Hydra). We could create simple authorization server endpoints that would do just enough so we can test this, eventually.

@scholzj
Copy link
Member

scholzj commented Feb 20, 2020

@mstruk So we do not use the scope for anything in the broker part?

@mstruk
Copy link
Contributor

mstruk commented Feb 20, 2020

@scholzj Correct. Authentication part does not use it - it's more for authorization use, but there SimpleACL isn't aware of it, and KeycloakRBACAuthorizer doesn't make use of it either - in favor of fine grained security provided by Authorization Services.

@scholzj
Copy link
Member

scholzj commented Feb 20, 2020

Thanks for the PR.

@scholzj scholzj merged commit 2f5ee7f into strimzi:master Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants