fix: oidc does not require a method in the payload

[Benehiko]

The oidc strategy does not validate the method set in the payload on registration or login.
This recently caused issues with custom UIs sending data to Kratos which do not contain method: oidc or contains a different method e.g method: password but with a provider: X set.

This produced unexpected behavior in Kratos, which returns a status code 422 and attempts to redirect the user to the google sign in flow, even though the registration was a POST request with the method: password.

Since the regression has been in the code a while, this PR does not attempt to fix the issue directly, but rather includes tracing and proper fallback handling to users that actually do include a method: X in the POST payload.

For example:

1. POST with method: password and traits and provider: google
2. Since the method is password, we continue to the password strategy
3. We log that the provider was specified and that the method was something other than oidc.

This PR does not fix requiring the method: oidc to be set for an OIDC flow, due to the regression existing for so long. The oidc regression happened in this commit from 2021 and will thus be phased out gradually.
f5091fa

Related issue(s)

Checklist

• I have read the contributing guidelines.
• I have referenced an issue containing the design document if my change
  introduces a new feature.
• I am following the
  contributing code guidelines.
• I have read the security policy.
• I confirm that this pull request does not address a security
  vulnerability. If this pull request addresses a security vulnerability, I
  confirm that I got the approval (please contact
  [email protected]) from the maintainers to push
  the changes.
• I have added tests that prove my fix is effective or that my feature
  works.
• I have added or changed the documentation.

Further Comments

[aeneasr]

Could we not just require the OIDC method to skip execution if (method == "oidc" || method == "") && provider != ""?

[Benehiko]

Could we not just require the OIDC method to skip execution if (method == "oidc" || method == "") && provider != ""?

So the problem here is that the OIDC method doesn't check the payload for the method. In fact it doesn't even get this value marshalled into the struct atm. So you can do an OIDC flow by just submitting { "provider" : "google" } and nothing else. This means that some users of our APIs might assume this to be the correct behavior and just have implemented their UIs sending only the minimal payload without a method value. So updaing this now to require the method to be set as oidc would most likely break existing UIs.

In an edge case, some users might be sending us a payload containing a method and the provider value. e.g.

{
  "method": "password",
  "password": "1234",
  "traits.email": "[email protected]",
  "provider": "google"
}

In the past the password method was the first strategy in the strategies array, but after b2b sso we have oidc as the first strategy which causes the OIDC strategy to just continue execution (it ignores method: password). This now broke a customers UI by accident, even though our APIs technically should not have changed.

In this PR I address the following:

1. On method != "" && method != "oidc" break out of this strategy and continue to the next
2. Add warning logs for us if the step 1 condition applies and a provider property was set. (this allows us to contact customers affected)
3. Add a way for the OIDC method to actually extract the method from the payload (it's currently ignored).

TL;DR I have added the method=oidc && method =="" condition to the PR already which continues the flow to a following flow after OIDC if the condition is not met.

[codecov]

Codecov Report

Merging #3564 (21de034) into master (e16fed1) will increase coverage by 0.01%.
Report is 4 commits behind head on master.
The diff coverage is 71.42%.

❗ Current head 21de034 differs from pull request most recent head ced735e. Consider uploading reports for the commit ced735e to get more accurate results

@@            Coverage Diff             @@
##           master    #3564      +/-   ##
==========================================
+ Coverage   78.13%   78.15%   +0.01%     
==========================================
  Files         341      341              
  Lines       22923    22939      +16     
==========================================
+ Hits        17912    17927      +15     
- Misses       3664     3667       +3     
+ Partials     1347     1345       -2     

Files	Coverage Δ
selfservice/hook/web_hook.go	79.65% <100.00%> (ø)
selfservice/strategy/oidc/strategy_registration.go	69.29% <100.00%> (+0.97%)	⬆️
identity/handler.go	84.09% <33.33%> (ø)
cmd/daemon/serve.go	87.93% <75.00%> (ø)
selfservice/strategy/oidc/strategy_login.go	68.37% <47.05%> (-3.85%)	⬇️

... and 2 files with indirect coverage changes

[aeneasr]

Thank you! I contextualized the things you correctly identified:

Could we not just require the OIDC method to skip execution if (method == "oidc" || method == "") && provider != ""?

So the problem here is that the OIDC method doesn't check the payload for the method. In fact it doesn't even get this value marshalled into the struct atm. So you can do an OIDC flow by just submitting { "provider" : "google" } and nothing else.

Correct, the reason for this is how HTML forms work. Basically, the form looks like this:

<form action="self-service/login/...">
<input type="submit" name="provider" value="github">
<input type="submit" name="provider" value="google">
<input type="submit" name="provider" value="google">

<input type="text" name="password">
<input type="submit" name="method" value="password">

So when the user clicks on the submit button, the form will submit the form with provider="google". Unfortunately, I could not find a way to get it to submit method=oidc&provider=google with raw HTML without needing two clicks (click on e.g. a dropdown menu or radio box, then click submit).

This means that some users of our APIs might assume this to be the correct behavior and just have implemented their UIs sending only the minimal payload without a method value. So updaing this now to require the method to be set as oidc would most likely break existing UIs.

Agreed, forcing the need for method="oidc" will break the above case, which in fact I believe will break most of the HTML-based integrations including our AX. Hence the idea of (method == "oidc" || method == "") && provider != "" where method == "" && provider != "" covers this case.

In an edge case, some users might be sending us a payload containing a method and the provider value. e.g.

{
  "method": "password",
  "password": "1234",
  "traits.email": "[email protected]",
  "provider": "google"
}

Paraphrasing: In this scenario the user expects that we use the password method because "method" is the deciding discriminator for action. I think that makes sense!

In the past the password method was the first strategy in the strategies array, but after b2b sso we have oidc as the first strategy which causes the OIDC strategy to just continue execution (it ignores method: password). This now broke a customers UI by accident, even though our APIs technically should not have changed.

In this PR I address the following:

1. On method != "" && method != "oidc" break out of this strategy and continue to the next
2. Add warning logs for us if the step 1 condition applies and a provider property was set. (this allows us to contact customers affected)
3. Add a way for the OIDC method to actually extract the method from the payload (it's currently ignored).

TL;DR I have added the method=oidc && method =="" condition to the PR already which continues the flow to a following flow after OIDC if the condition is not met.

I think this is the correct solution :) Will review now again!

[aeneasr]

I think this is the correct solution, I just have a two comments on the log messages and deprecation notice

[zepatrik]

Oh I see, now I finally understand why we had this weird behavior in the first place.