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

Captcha is incompatible with SSO #1334

Open
tbouliere-datasolution opened this issue Dec 2, 2022 · 9 comments
Open

Captcha is incompatible with SSO #1334

tbouliere-datasolution opened this issue Dec 2, 2022 · 9 comments
Labels
bug Something isn't working wait Waiting for something (e.g. new ICM release)

Comments

@tbouliere-datasolution
Copy link
Contributor

tbouliere-datasolution commented Dec 2, 2022

When using the Google Recaptcha V3 service, the captcha verification information is transmitted to the icm using the Authorization header.

headers.set(ApiService.AUTHORIZATION_HEADER_KEY, `CAPTCHA recaptcha_token=${captcha} action=${captchaAction}`)

This conflicts with the standard SSO operation which uses this header to pass a bearer token

headers: req.headers.set(ApiService.AUTHORIZATION_HEADER_KEY, `Bearer ${this.oauthService.getIdToken()}`),

Expected Behavior

Use a custom header. For instance X-Captcha-Token.
And remember to modify the corresponding class on the icm side: com.intershop.sellside.rest.common.internal.auth.CaptchaAuthenticationProvider

Steps to Reproduce the Bug

Steps to reproduce the behavior:

  1. Login with any sso provider
  2. Post a form with a captcha that points to an endpoint with required authentication.
  3. Eat popcorn while enjoying the beautiful side iffect

Environment Details

  • Any

Additional Context, like Screenshots, Log File Snippets etc.

AB#81572

@tbouliere-datasolution tbouliere-datasolution added the bug Something isn't working label Dec 2, 2022
@shauke
Copy link
Collaborator

shauke commented Dec 5, 2022

@tbouliere-datasolution Can you please provide more details for the problematic use case?
What form needs an additional Captcha authorization once the user is logged in? Usually only form submissions for unregistered users need Captcha protection so you should never have a combination once you are logged in via SSO.

@shauke shauke self-assigned this Dec 5, 2022
@tbouliere-datasolution
Copy link
Contributor Author

Captchas are mainly used to prevent spamming.
If your website contains a public registration feature, any robot can create an account and then use a form that triggers an email to be sent.

It is a very strong assumption to limit captchas to non-registered users.

@tbouliere-datasolution
Copy link
Contributor Author

For example, you can take the function "send the product to a friend".
It is better to add a captcha at this stage, even for registered user.

@shauke
Copy link
Collaborator

shauke commented Dec 7, 2022

Captchas are mainly used to prevent spamming. If your website contains a public registration feature, any robot can create an account and then use a form that triggers an email to be sent.

It is a very strong assumption to limit captchas to non-registered users.

Well the registration is exactly an example for a non-registered user interaction that is protected by a captcha in the PWA if it is not done with SSO.
And if you use an SSO provider for the registration then the verification of real users is handled by the SSO provider. Once the user comes back to the PWA the user is already authenticated.

@shauke
Copy link
Collaborator

shauke commented Dec 7, 2022

For example, you can take the function "send the product to a friend". It is better to add a captcha at this stage, even for registered user.

The current implementation for "Email a friend" uses the users mail program exactly to prevent spam being send from the PWA/ICM. In the standard there is nothing that we could protect with an additional Captcha here.

But I will talk the according REST API team to check whether they see any use case that requires a combination of both authentication methods for one request.

@tbouliere-datasolution
Copy link
Contributor Author

tbouliere-datasolution commented Dec 7, 2022

Ok I see your point @shauke.
The general idea is to say that if the user is logged in, then it is a human and therefore the captcha is unnecessary in some way.

However I still think that a catcha can be necessary, on pages that need to be accessible for both logged in and not logged in users.

For example for the feature, contact-us (https://intershoppwa.azurewebsites.net/contact)
This page introduces a hard-coded captcha :

and a backend coded check :

if (this.authenticationProvider != null
		&& this.authenticationProvider instanceof CaptchaAuthenticationProvider) {
	final CaptchaAuthenticationProvider cap = (CaptchaAuthenticationProvider) this.authenticationProvider;
	if (cap.isCaptchaRequired("intershop.CaptchaPreferences.ContactUs")) {
		cap.authenticate(this.currentRequest, this.currentResponse);
	}
}

And you got a probleme there. Either you activate the captcha feature and you protected non login pages, but it crash when you have an SSO BearerToken, or you desactivate the captcha for everyone.

This is why I think that captcha should not be treat as an authentification state.

@shauke shauke added the wait Waiting for something (e.g. new ICM release) label Dec 14, 2022
@shauke
Copy link
Collaborator

shauke commented Dec 14, 2022

After discussion with the responsible ICM REST API team it was decided to implement the suggestion to expect the Captcha token at an alternative header key (with fallback to the ApiService.AUTHORIZATION_HEADER_KEY for backwards compatibility). Once an ICM release with this change is available the PWA implementation will be adapted accordingly.

@tbouliere-datasolution
Copy link
Contributor Author

Nice !

@shauke
Copy link
Collaborator

shauke commented Feb 28, 2023

Adapted headers available with ICM 7.10.40 and ICM 11.0.12.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working wait Waiting for something (e.g. new ICM release)
Projects
None yet
Development

No branches or pull requests

2 participants