fix: allow patching some /credentials sub-paths

[hperl]

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

[jonas-jonas]

Maybe we should also have tests that check, that it's not possible to override nested objects by specifying the wrong path.

E.g.

PATCH 
{
	"op": "replace",
	"path": "/credentials/password",
	"value": {
		"hashed_password": "<hash>"
	},
}

Here, /config/ is missing in the path. Which seems like an easy mistake to make, but would render the identity unusable? Or is this caught by the validator?

[hperl]

Maybe we should also have tests that check, that it's not possible to override nested objects by specifying the wrong path.

E.g.

PATCH 
{
	"op": "replace",
	"path": "/credentials/password",
	"value": {
		"hashed_password": "<hash>"
	},
}

Here, /config/ is missing in the path. Which seems like an easy mistake to make, but would render the identity unusable? Or is this caught by the validator?

Invalid keys are ignored, and AFAIK this is how it has always been.

I agree that this would be useful, and also to extend the PATCHing to OIDC, but here I'd just like to revert to the previous behaviour first (with the added benefit of a test). Let's move this into a new issue?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.59%. Comparing base (44eb305) to head (e57e201).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4277      +/-   ##
==========================================
+ Coverage   78.57%   78.59%   +0.01%     
==========================================
  Files         381      381              
  Lines       27376    27376              
==========================================
+ Hits        21511    21516       +5     
+ Misses       4241     4237       -4     
+ Partials     1624     1623       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jonas-jonas]

Invalid keys are ignored, and AFAIK this is how it has always been.

Not what I meant. The key is technically valid, because the source JSON object the patch is applied against, has the following structure:

{
  "credentials": {
    "password": {
	  "config": {
		"hashed_password": "<hash>",
      }
	}
  }
}

But after applying the faulty patch, the JSON document looks like this:

{
  "credentials": {
    "password": {
	   "hashed_password": "<hash>",
	}
  }
}

Which is not valid, but I am not sure what would happen if you send the request. So a test for that case would be good. Again, I think that is a very common scenario that will happen easily, and we should do our best to prevent it.

[alnr]

this line of code is still broken and doesn't do what the comment says. pls fix 🙏

kratos/identity/handler.go

Line 932 in e57e201

patchedIdentity.Credentials = credentials

[aeneasr]

Ideally, for both create, patch, and PUT operations, the following should be implemented with proper
validation:

1. Passwords:

   • Set or change passwords (with validation ensuring the hash or plaintext password is correct).
   • Delete passwords.

2. OIDC:

   • Set or change OIDC subject and provider.
   • Delete OIDC subject and provider.

3. WebAuthn and Passkey Credentials:

   • Set, change, or delete WebAuthn and passkey credentials (e.g., to import passkey users).

4. Code Credentials:

   • Activate or deactivate code credentials.

5. TOTP:

   • Set, change, or delete TOTP configurations.

6. Lookup Secrets:

   • Set, change, or delete lookup secrets.

Restrictions:
What should not be allowed is:

• Adding arbitrary keys to the config object.
• Changing identifiers that are typically set by the system.
• Modifying keys that should only be set by the system.

I opened #4279 for a proper fix. In the meantime, reverting the forbidden paths is not more wrong / broken than what we currently have, so I'd rather have this merged right now to unblock customers.

[hperl]

I opened #4279 for a proper fix. In the meantime, reverting the forbidden paths is not more wrong / broken than what we currently have, so I'd rather have this merged right now to unblock customers.