Add Slack well-known social OIDC provider

[michalvavrik]

• Closes: Add Slack to the list of supported OIDC providers  #38462

[github-actions]

🙈 The PR is closed and the preview is expired.

[sberyozkin]

Hi @michalvavrik , thanks, LGTM, I'll be suggesting a few minor updates, the first one is to draw red lines over the client secret, and id as well, even though that was a test app,
I'm travelling, will add a few comments later, thanks very much

[sberyozkin]

@michalvavrik Yes, in all other images we do highlight that this is a sensitive info...

I have a few more comments about the way the Slack app setup is suggested now.

Please follow the manual setup option, JSON there looks complex to me.
Also, I don't think presetting scopes at the start up time is what we should recommend. Typically users will approve scopes dynamically, and presetting them would also block overriding the scopes property at the quarkus level, for example, someone will wan5 to avoid sharing an email, but the registered app will allow it. Or if someone will want an extra scope requested at the quarkus level.

The other suggestion is cosmetic, please use Quarkus Slack as the app or workspace name, just to keep it consistent with the recent updates to Github.

If Slack insists on the https scheme then we should offer an ngrok based url, similar to how Steph did it for ex for Facebook. Also, I propose to have URI ending with /slack to make it consistent with other 2 recent PRs.

[sberyozkin]

Hi Michal, left a few more comments, they are really rather cosmetic, minor ones, sorry this issue is taking more time than expected

[michalvavrik]

I have deleted all my comments and I'll address every single request for change.

[sberyozkin]

Hi @michalvavrik, first of all, thanks for doing what I should've done ages ago, by adding the provider specific Wiremock support, starting from Slack, it really all looks quite perfect,
I'm only trying for us to get the simplest possible setup so that we can easily explain it to users, for them copy and paste individual test classes, etc

LGTM, thanks

[sberyozkin]

Hi Michal, @michalvavrik, LGTM, thanks for the effort, it all looks great, I'll do a single push myself, to avoid distracting you as you are very busy, related to scopes (vs required claims), and will merge afterwards, thanks

[sberyozkin]

Please don't merge yet, I'll do a minor update, to replace required-claims with scopes

[michalvavrik]

Thanks @sberyozkin , yeah, I have other things ATM and I'll be happy when this gets in. Go ahead.

[sberyozkin]

@michalvavrik I pushed a minor update: scope is set at the provider definition, and I dropped @Tenant("slack") to get the convention based resolver working. Interestingly, the custom discovery stub is missed at the startup, 404 is reported, so I thought of moving the tenant definition to the dynamic resolver, but then decided to keep it to test the recovery...

Then I followed up with a unit test update to show users can override the scope with a more restricted value if needed

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 5a05d9a.

✅ 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.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 5a05d9a.

✅ 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.

You can consult the Develocity build scans.