feat: URL client id for client credentials by elf-pavlik · Pull Request #2041 · CommunitySolidServer/CommunitySolidServer

elf-pavlik · 2025-06-26T22:45:41Z

📁 Related issues

✍️ Description

It allows to use URL as client_id when creating a client credential.
I only uses uuid if no name was provided
If there is an existing credential with provided name it responds with an error - is that a breaking change?
This approach makes sense to me, if one doesn't care about specific name they can just leave it out and get random uuid.
I had to change order of OIDC adapters to try registered client credential before fetching client id
TBH I'm not sure if this is a new feature or a fix or major

✅ PR check list

Before this pull request can be merged, a core maintainer will check whether

this PR is labeled with the correct semver label
- semver.patch: Backwards compatible bug fixes.
- semver.minor: Backwards compatible feature additions.
- semver.major: Breaking changes. This includes changing interfaces or configuration behaviour.
the correct branch is targeted. Patch updates can target main, other changes should target the latest versions/* branch.
the RELEASE_NOTES.md document in case of relevant feature or config changes.
any relevant documentation was updated to reflect the changes in this PR.

joachimvh

If there is an existing credential with provided name it responds with an error - is that a breaking change?

Technically yes, but it seems minor enough that we can cheat and not do a major release for this. Same for the ordering of the adapters changing, should it really be needed.

Alternatively it could also be an option to add an optional flag to the class to change the behavior.

src/identity/interaction/client-credentials/CreateClientCredentialsHandler.ts

joachimvh · 2025-06-27T06:31:48Z

test/deploy/createAccountCredentials.ts

  res = await fetch(controls.account.clientCredentials, {
    method: 'POST',
    headers: { authorization, 'content-type': 'application/json' },
-    body: JSON.stringify({ name: 'token', webId }),


Was there a specific reason to change this?

This one failed https://github.com/CommunitySolidServer/CommunitySolidServer/actions/runs/15914010572/job/44887876671
Due to new error I added: "Token with this name already exists."
This error message itself could include a hint to omit the name for auto generated uuid

This makes me think that perhaps we should be more careful about changing the default behaviour here, as this update would break similar setups that might exist. Additionally, the uniqueness check of the label leaks information about which labels exist of other users, which might be a concern in some situations. So I would suggest adding an optional constructor parameter to enable this behaviour (verbatimLabels?), which defaults to false, so existing setups don't change. The configuration generator could then have another option to enable this (I can do this part after this PR is merged). The uniqueness check then only happens when this parameter is set to true. In case it is set to false the uuid is always added.

Maybe it would make more sense to handle uuid based client ids and URL client ids separately. Especially if there was a verification step #2041 (comment)

I revereted commit with this change

joachimvh · 2025-06-27T06:52:18Z

config/identity/handler/base/adapter-factory.json

-      "@type": "ClientCredentialsAdapterFactory",
-      "webIdStore": { "@id": "urn:solid-server:default:WebIdStore" },
-      "clientCredentialsStore": { "@id": "urn:solid-server:default:ClientCredentialsStore" },
+      "@type": "ClientIdAdapterFactory",
+      "converter": { "@id": "urn:solid-server:default:RepresentationConverter" },
      "source": {
-        "@type": "ClientIdAdapterFactory",
-        "converter": { "@id": "urn:solid-server:default:RepresentationConverter" },
+        "@type": "ClientCredentialsAdapterFactory",
+        "webIdStore": { "@id": "urn:solid-server:default:WebIdStore" },
+        "clientCredentialsStore": { "@id": "urn:solid-server:default:ClientCredentialsStore" },


What is the reason these needed to be swapped? I'm guessing it has something to do with the other adapter trying to fetch the URL label maybe but can you tell me exactly what goes wrong?

Without that change ClientIdAdapterFactory was trying to fetch that URL, I don't remember if there was a problem if it didn't exist, but when it was able to fetch that ClientID, it was setting token_endpoint_auth_method to none and complaining that it wasn't allowed and it didn't pick credentials passed via Authorization hearder.

I think it doesn't make sense to use ClientIDAdapter at all when ClientCredentialsAdapter handles that client properly, that's why in the end I changed the order. As you can see all other tests are still passing so it doesn't seem to be affecting anything elese, and if that client hasn't been registered for client credential for that webid there should be no problem.

I haven't tested what happens if that user tries to use the same client both with client credential and with authorization code. Should I add integration test for that?

The ClientIdAdapterFactory was written under the assumption that if the incoming ID is a URL, this is always the class that should handle it, but that breaks here. The "deeper" class always triggers first, so if a client ID is used that is also a credential label the credential adapter will trigger first. But this could potentially cause issues when someone uses a client ID request which happens to also be the label of a credential token (of someone else potentially). So this definitely needs to be investigated what would happen there. I'm not sure if this would work with how the adapters currently work.

Since only one client credential per Client ID is supported with this approach (#2053 could be an alternative) I think a check similar to linking an existing WebID would make sense here. One would have to prove control over client to get a URL Client ID credential for it. Once such credential is created, it can only be used with it and no more authorization code flow for that client.

joachimvh · 2025-06-27T07:43:23Z

test/integration/Identity.test.ts

    });
  });

+  describe('using client_credentials - url', (): void => {


Why was this entire block added as it differs on only one line from the previous block. Is it to check for an issue related to the reason the order of the components was changed?

Yes, just changing from string to URL was resulting in a different adapter handing it and failing. I though the safest would be to run the whole describe for both cases to make sure that both string and url are fully functional.

Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>

elf-pavlik · 2025-08-14T17:19:02Z

I created a discussion in oidc-provider repo

Support: urn:ietf:params:oauth:grant-type:jwt-bearer panva/node-oidc-provider#1348

elf-pavlik · 2025-08-17T00:00:35Z

superseded by #2053

elf-pavlik · 2025-10-31T00:16:10Z

Since I didn't find anyone else that would be interested in #2053 and changes here would unblock my immediate need, I think I'd better get this one done and keep looking if anyone else finds use for grant-type:jwt-bearer

This reverts commit 7eaf6c3.

elf-pavlik · 2025-11-12T19:22:57Z

New approach handles string and URL clientId differently. For string it should be the same as before and for URL it requires ownership validation using the same one as for linking WebID.

Since registering client credential for URL will not allow its use with Authorization Code flow, we could add a small warning box in the dev documentation.

TallTed · 2025-11-12T19:37:32Z

src/identity/interaction/client-credentials/CreateClientCredentialsHandler.ts

+    let label: string;
+
+    if (name && isUrl(name)) {
+      // Validate owneship first not to leak existence of credentials!


Suggested change

// Validate owneship first not to leak existence of credentials!

// Validate ownership first, so as not to leak existence of credentials!

Thanks! I will most likely need to add a test case for that so I'll include your suggestion in that commit.

elf-pavlik added 3 commits June 25, 2025 14:57

feat: client credential id can be a URL

091ac9f

fix: change order of OIDC adapters

86f70ba

chore: use random name for client credential in test/deploy

7eaf6c3

joachimvh reviewed Jun 27, 2025

View reviewed changes

fixup! feat: client credential id can be a URL

b0bdff0

Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>

elf-pavlik mentioned this pull request Jun 30, 2025

[UC] Server Side authentication and Data Access w3c/lws-ucs#39

Open

elf-pavlik marked this pull request as draft August 14, 2025 21:46

elf-pavlik mentioned this pull request Aug 17, 2025

grant-type:jwt-bearer similar to client credentials but with proper support for URL client_id #2053

Draft

10 tasks

elf-pavlik closed this Aug 17, 2025

elf-pavlik reopened this Oct 31, 2025

elf-pavlik added 2 commits November 12, 2025 13:06

fix: URL ClientID uses ownership verification

3a2d9d7

Revert "chore: use random name for client credential in test/deploy"

e3b6292

This reverts commit 7eaf6c3.

elf-pavlik marked this pull request as ready for review November 12, 2025 19:23

TallTed reviewed Nov 12, 2025

View reviewed changes

This was referenced Nov 12, 2025

Authorization server behaviour for Client-Credentials Grant flow solid/solid-oidc#75

Open

LWS Authentication w3c/lws-protocol#43

Merged

elf-pavlik mentioned this pull request Feb 15, 2026

feat: add client credentials as a MUST solid/solid-oidc#245

Open

	// Validate owneship first not to leak existence of credentials!
	// Validate ownership first, so as not to leak existence of credentials!

Conversation

elf-pavlik commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📁 Related issues

✍️ Description

✅ PR check list

Uh oh!

joachimvh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elf-pavlik Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elf-pavlik Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elf-pavlik Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elf-pavlik Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elf-pavlik commented Aug 14, 2025

Uh oh!

elf-pavlik commented Aug 17, 2025

Uh oh!

elf-pavlik commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elf-pavlik commented Nov 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elf-pavlik commented Jun 26, 2025 •

edited

Loading

elf-pavlik Jun 27, 2025 •

edited

Loading

elf-pavlik Oct 31, 2025 •

edited

Loading

elf-pavlik Jun 27, 2025 •

edited

Loading

elf-pavlik Jun 27, 2025 •

edited

Loading

elf-pavlik commented Oct 31, 2025 •

edited

Loading