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

Enhancement: OAuth2TokenProvider for Authentication Against Self-Hosted Camunda Stack (503) #607

Conversation

LennartKleymann
Copy link
Contributor

Hey! 👋 I've just introduced an OAuth2TokenProvider to facilitate authentication against a self-hosted zeebe stack. I'd love to hear your thoughts and feedback on this addition.

Changelog:

OAuth2TokenProvider:

  • Introduced OAuth2TokenProvider for seamless authentication.
  • Implemented OAuth2TokenProviderBuilder for flexibility in configuration.
  • Moved AccessToken to the Misc Namespace and Folder for better organization.
  • Extracted duplicated code from OAuth2TokenProvider and CamundaCloudTokenProvider into a common base class for improved maintainability.

Integration Tests:

  • Enabled the initiation of integration tests with an identity zeebe stack.
  • Added integration test to authenticate against the identity zeebe stack

Unit Tests:

  • Implemented thorough unit tests for both OAuth2TokenProvider and OAuth2TokenProviderBuilder to ensure robust functionality.

Looking forward to your valuable feedback!

@CLAassistant
Copy link

CLAassistant commented Nov 17, 2023

CLA assistant check
All committers have signed the CLA.

@ChrisKujawa
Copy link
Collaborator

Hey @LennartKleymann

first of all thank you very much for your time and effort and creating this contribution. I really appreciate that!

Unfortunately, I have limited time to maintain this project, so please bear with me.

One thing I already saw that might be great to do from your side:

  1. Please sign the CLA otherwise I can't accept your PR.
  2. Please follow our commit guidelines https://github.com/camunda/zeebe/blob/main/CONTRIBUTING.md#commit-message-guidelines
  3. As I wrote earlier I haven't looked into it, but such large PRs always mean that it takes longer to review than smaller ones, as it is often harder to understand and reason about. If possible please split them up into smaller ones, of course only if you think it makes sense.

I try my best to review it soon.

@LennartKleymann
Copy link
Contributor Author

Hi @Zelldon,

I've completed the signing of the CLA and have divided my extensive commit into smaller ones, adhering to the commit guidelines. While I understand the practice of splitting pull requests (PRs), I believe consolidating them into a single PR is more suitable in this case, as it encompasses a cohesive feature.

@fabianschurz
Copy link

Would love to see this PR being merged, i'm missing this feature and need it in order to remove a dirty workaround.

@ChrisKujawa
Copy link
Collaborator

I try to review asap but zeebe project and other thing keep me busy :)

@fabianschurz maybe you want to give feedback from the user perspective if this would be something you would like to see and use like it is rjght now already.

@fabianschurz
Copy link

fabianschurz commented Nov 28, 2023

@Zelldon
I reviewed the code, i don't have any improvements or additions. LGTM, it's implemented the same way the cloud provider is implemented at the moment. So for me this PR would be suitable and would exactly fit for my use case. I like the new BaseTokenProvider which gives the flexibility to add even more options, f.ex. if something different than keycloak (/OAuth2) could be used in the future.

@FJuette
Copy link

FJuette commented Dec 15, 2023

I would love to see this PR accepted. After upgrading my local camunda-stack it was a nightmare to get authentication done right (before I made unauthorized requests). Thanks to this PR I have a workaround for token retrieval.

@ChrisKujawa
Copy link
Collaborator

Hey, thanks all for your interest in this PR and especially to @LennartKleymann for contributing. I reiterated over it and created #625 (which is merged now). I will release a new version soon.

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.

5 participants