-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add Twitch OIDC Provider #29244
Add Twitch OIDC Provider #29244
Conversation
Thanks for your pull request! The title of your pull request does not follow our editorial rules. Could you have a look?
|
Hi @biswassri Glad you've found the time to go ahead and open this PR, nice work. I'm assuming it is still |
@sberyozkin my sincere apologies for the delay in progress on this due to holidays. Thank you for your review. |
Hi @biswassri Np at all, and apologies I missed your question yesterday - we appreciate very much you trying to complete this PR, great effort 👍 . When you confirm it is working, then make a few images showing a process of logging in to Twitch and update the Quarkus doc where all other providers are listed. Thanks |
I'd suggest to add a new test to oidc-wiremock, you don't have really do too much, just full-text search all that belongs already tested trusted provider in oidc-wiremock modole (e.g. we do for github, but c&p won't do there :/): https://github.com/quarkusio/quarkus/blob/main/integration-tests/oidc-wiremock/src/test/java/io/quarkus/it/keycloak/BearerOpaqueTokenAuthorizationTest.java#L114 https://github.com/quarkusio/quarkus/blob/main/integration-tests/oidc-wiremock/src/main/java/io/quarkus/it/keycloak/CustomTenantResolver.java#L35 and adjust mocking to Twitcher endpoints. Thing is that OidcUtilTest don't test whole flow and if you test it yourself and someone else add breaking change, no test will fail. |
@michalvavrik We don't really do Github tests in wiremock since the OIDC code supporting the connection to GitHub is in fact not GitHub specific. If GitHub changes something these wiremock tests won't help. IMHO to keep it simpler, adding a a couple of simple tests confirming |
I understand the concept of mocking, but with end to end test that mocks current Twitch state, CI verifies that changes in Quarkus did not affect it. Otherwise you just trust you did everything right (in the future), so it must works. Anyway, I don't really see a point to discuss it, I accept your arguments and keep my fingers crossed it works. |
@michalvavrik As I said,
Well you started a discussion and I tried to support it with my comments.
If we test that these |
I propose to close this PR as it has been open for a while, @biswassri thanks for trying to complete it, can you confirm you can proceed and get to the completion ? I'd ask someone else who is interested if you have no time now, np, let me know please |
@sberyozkin Just saw your comment. Apologies for the delay haven't had any time to progress on this. Thank you for waiting on me. Allow me to give it a try once more time this week. If I'm not able to close this out by this weekend, please feel free to close this PR and re-assign the issue. Thanks again! |
@biswassri Hi, no problems at all, lets give it another 2 weeks, just to summarize what I hope to see being added to the PR:
And that is it. Thanks |
Hi @gastaldi , would you be interested to finalize this PR next week if it has not been completed before then ?:I'm assuming you have a Twitch account already setup? May be we can do some demo afterwards... |
@sberyozkin hey, I might be able to have a look at it next week, but no promises 😀 |
@gastaldi sure, I meant more like starting from the next week as we agreed here to wait for another couple of weeks , so please take your time , next few weeks or so would be grand 😀 |
@biswassri thanks for giving it a try, we do appreciate it, you will be welcome to pick up another issue once you find more time, thanks |
Thanks @sberyozkin ! Apologies for not being able to complete this. I think I picked up more than I could chew with this particular task. Thanks again. |
@gastaldi Please feel free to continue on this PR or close it out. |
Thanks @biswassri, hope to see you contributing in the future again, thanks |
Hey @gastaldi I plan to check if I can use this provider in demos so let me have a look at finalizing this PR a bit later, will ask you to review :-) |
@gastaldi This PR is now ready for review, I have verified it with my local demo. |
@FroMage FYI |
I might follow up with some minor updates |
Cool, let me follow the instructions and test it with a Twitch account |
🙈 The PR is closed and the preview is expired. |
Hey @gastaldi , you can have |
This comment has been minimized.
This comment has been minimized.
@sberyozkin I keep getting |
@gastaldi I was getting it before realizing the provider had to enforce https redirect scheme. Can you please compare url encoded |
Your app can have a large uri space, for testing I setup a single method endpoint and registered that endpoint method address, replacing local host and port with the ngrok one. If you want to secure a larger space, add |
This comment has been minimized.
This comment has been minimized.
extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java
Outdated
Show resolved
Hide resolved
Simplify Twitch configuration Co-Authored-By: Sergey Beryozkin <[email protected]>
✔️ The latest workflow run for the pull request has completed successfully. It should be safe to merge provided you have a look at the other checks in the summary. |
Changed the Client secret and the URL and it works now. Let's merge this. Thanks for your support! |
Fixes #24833