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

Add Twitter OIDC provider configuration #24774

Merged
merged 3 commits into from
Apr 6, 2022
Merged

Conversation

FroMage
Copy link
Member

@FroMage FroMage commented Apr 5, 2022

Still missing:

  • docs (I didn't find where the current list is documented in our guides, did we forget documenting it?)
  • tests

Fixes #23593
Fixes #14093

@sberyozkin
Copy link
Member

sberyozkin commented Apr 5, 2022

Hey @FroMage, thanks

Re docs, I think it would be great for the related Renarde docs adapted for Quarkus as well, as discussed at #21521 (assigned to you), perhaps you can have a look at it next ? ( we can also check then if something else has to be optimized in Quarkus)

Re the tests - just copy and paste a pair of unit tests in OidcUtilsTest (github or facebook related and have 2 new ones for twitter), that should be enough

@sberyozkin
Copy link
Member

@FroMage LGTM thanks, I've suggested a minor clarification to the new configuration property; please add 2 tests to OidcUtilsTest and it will be ready to go

@sberyozkin
Copy link
Member

May be worth rebasing as there is a message This branch is out of date

@gsmet
Copy link
Member

gsmet commented Apr 5, 2022

May be worth rebasing as there is a message This branch is out of date

No need for rebasing if the branch is not too old and there are no conflicts. You can do it when you push new fixes but let's not rebase just for the sake of it (see my email on quarkus-dev about this rebase feature and CI resources).

In this case, you will push new things so you can rebase when doing it. Talking about the general case here.

@FroMage FroMage marked this pull request as ready for review April 6, 2022 12:38
@FroMage
Copy link
Member Author

FroMage commented Apr 6, 2022

OK, done.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

scopes.addAll(oidcConfigScopes);
configContext.oidcConfig.getAuthentication().scopes.ifPresent(scopes::addAll);
Copy link
Member

@sberyozkin sberyozkin Apr 6, 2022

Choose a reason for hiding this comment

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

@FroMage I'm not sure why, as it does look like it duplicates the previous line, but apparently removing it causes a test failure, can you check it please

Copy link
Member

Choose a reason for hiding this comment

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

@FroMage Checking now if removing this line is a culprit

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, passed for me

Copy link
Member

@sberyozkin sberyozkin Apr 6, 2022

Choose a reason for hiding this comment

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

@FroMage Your code is perfectly fine, the test was broken a bit (well, I did not realize till now the scopes were duplicated - this should never be a problem but is obviously redundant and can indeed cause some problems, thanks)

@sberyozkin sberyozkin self-requested a review April 6, 2022 17:22
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

Thanks @FroMage for spending the time on testing it with Twitter

@sberyozkin sberyozkin merged commit 3947967 into quarkusio:main Apr 6, 2022
@quarkus-bot quarkus-bot bot added this to the 2.9 - main milestone Apr 6, 2022
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Apr 6, 2022
@gsmet gsmet modified the milestones: 2.9 - main, 2.8.1.Final Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/oidc kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add OIDC Twitter provider configuration Skip OIDC 'openid' scope redirect parameter
3 participants