feat: URL client id for client credentials #2041
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,6 @@ | |
| <ol id="webIdList"> | ||
| </ol> | ||
| </fieldset> | ||
| <ul class="actions"> | ||
| <li><button type="submit" name="submit" disabled>Create token</button></li> | ||
| <li><button type="button" id="account-link">Back</button></li> | ||
|
|
@@ -66,11 +65,15 @@ | |
| } | ||
|
|
||
| addPostListener(async() => { | ||
| try { | ||
| const { id, secret } = await postJsonForm(controls.account.clientCredentials); | ||
| updateElement('token-id', id, { innerText: true }); | ||
| updateElement('token-secret', secret, { innerText: true }); | ||
| setVisibility('response', true); | ||
| setVisibility('mainForm', false); | ||
| } catch (err) { | ||
| setError(err.message, 'error') | ||
| } | ||
| }); | ||
| })(); | ||
| </script> | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,8 @@ import { | |
| import { fetch } from 'cross-fetch'; | ||
| import { parse, splitCookiesString } from 'set-cookie-parser'; | ||
| import type { App } from '../../src/init/App'; | ||
| import type { ResourceStore } from '../../src/storage/ResourceStore'; | ||
| import { BasicRepresentation } from '../../src/http/representation/BasicRepresentation'; | ||
| import { APPLICATION_X_WWW_FORM_URLENCODED } from '../../src/util/ContentTypes'; | ||
| import { joinUrl } from '../../src/util/PathUtil'; | ||
| import { register } from '../util/AccountUtil'; | ||
|
|
@@ -38,6 +40,7 @@ jest.spyOn(process, 'emitWarning').mockImplementation(); | |
| // We also need to parse the HTML in several steps since there is no API. | ||
| describe.each(stores)('A Solid server with IDP using %s', (name, { config, teardown }): void => { | ||
| let app: App; | ||
| let store: ResourceStore; | ||
| const redirectUrl = 'http://mockedredirect/'; | ||
| const container = new URL('secret/', baseUrl).href; | ||
| const oidcIssuer = baseUrl; | ||
|
|
@@ -63,7 +66,7 @@ describe.each(stores)('A Solid server with IDP using %s', (name, { config, teard | |
| 'urn:solid-server:default:variable:rootFilePath': rootFilePath, | ||
| }, | ||
| ) as Record<string, any>; | ||
| ({ app, store } = instances); | ||
| await app.start(); | ||
|
|
||
| // Create accounts | ||
|
|
@@ -339,7 +342,7 @@ describe.each(stores)('A Solid server with IDP using %s', (name, { config, teard | |
| }); | ||
| }); | ||
|
|
||
| describe('using client_credentials - string', (): void => { | ||
| const tokenUrl = joinUrl(baseUrl, '.oidc/token'); | ||
| let dpopKey: KeyPair; | ||
| let id: string | undefined; | ||
|
|
@@ -400,6 +403,111 @@ describe.each(stores)('A Solid server with IDP using %s', (name, { config, teard | |
| }); | ||
| }); | ||
|
|
||
| describe('using client_credentials - url', (): void => { | ||
|
Member
There was a problem hiding this comment. 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?
Contributor
Author
There was a problem hiding this comment. 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 |
||
| const tokenUrl = joinUrl(baseUrl, '.oidc/token'); | ||
| let dpopKey: KeyPair; | ||
| let id: string | undefined; | ||
| let secret: string | undefined; | ||
| let accessToken: string | undefined; | ||
| const publicContainer = joinUrl(baseUrl, '/public/'); | ||
| const clientId = joinUrl(publicContainer, 'client'); | ||
|
|
||
| beforeAll(async(): Promise<void> => { | ||
| dpopKey = await generateDpopKeyPair(); | ||
| }); | ||
|
|
||
| it('can request a credentials token.', async(): Promise<void> => { | ||
| // Create a public container where we can write any data | ||
| await store.setRepresentation( | ||
| { path: joinUrl(publicContainer, '.acl') }, | ||
| new BasicRepresentation( | ||
| `@prefix acl: <http://www.w3.org/ns/auth/acl#>. | ||
| @prefix foaf: <http://xmlns.com/foaf/0.1/>. | ||
| <#public> | ||
| a acl:Authorization; | ||
| acl:agentClass foaf:Agent; | ||
| acl:accessTo <./>; | ||
| acl:default <./>; | ||
| acl:mode acl:Read, acl:Write, acl:Control.`, | ||
| 'text/turtle', | ||
| ), | ||
| ); | ||
| // Create the ClientId | ||
| const createClientIdRes = await fetch(clientId, { | ||
| method: 'PUT', | ||
| headers: { 'content-type': 'text/turtle' }, | ||
| body: '', | ||
| }); | ||
| expect(createClientIdRes.status).toBe(201); | ||
|
|
||
| // Login and save cookie | ||
| const loginResponse = await fetch(controls.password.login, { | ||
| method: 'POST', | ||
| headers: { 'content-type': 'application/json' }, | ||
| body: JSON.stringify({ email, password }), | ||
| }); | ||
| const cookies = parse(splitCookiesString(loginResponse.headers.get('set-cookie')!)); | ||
| const cookie = `${cookies[0].name}=${cookies[0].value}`; | ||
|
|
||
| // Try to request token | ||
| const accountJson = await (await fetch(indexUrl, { headers: { cookie }})).json(); | ||
| const credentialsUrl = accountJson.controls.account.clientCredentials; | ||
| const failedTokenRes = await fetch(credentialsUrl, { | ||
| method: 'POST', | ||
| headers: { cookie, 'content-type': 'application/json' }, | ||
| body: JSON.stringify({ name: clientId, webId }), | ||
| }); | ||
|
|
||
| // Verification token is missing | ||
| expect(failedTokenRes.status).toBe(400); | ||
| const json = await failedTokenRes.json(); | ||
| expect(json.details?.quad).toBeDefined(); | ||
| const { quad } = json.details; | ||
| // Update the WebID with the identifying quad | ||
| const updateClientIdRes = await fetch(clientId, { | ||
| method: 'PUT', | ||
| headers: { 'content-type': 'text/turtle' }, | ||
| body: quad, | ||
| }); | ||
| expect(updateClientIdRes.status).toBe(205); | ||
|
|
||
| // Try again to request token | ||
| const tokenRes = await fetch(credentialsUrl, { | ||
| method: 'POST', | ||
| headers: { cookie, 'content-type': 'application/json' }, | ||
| body: JSON.stringify({ name: clientId, webId }), | ||
| }); | ||
| expect(tokenRes.status).toBe(200); | ||
| ({ id, secret } = await tokenRes.json()); | ||
| }); | ||
|
|
||
| it('can request an access token using the credentials.', async(): Promise<void> => { | ||
| const dpopHeader = await createDpopHeader(tokenUrl, 'POST', dpopKey); | ||
| const authString = `${encodeURIComponent(id!)}:${encodeURIComponent(secret!)}`; | ||
| const res = await fetch(tokenUrl, { | ||
| method: 'POST', | ||
| headers: { | ||
| authorization: `Basic ${Buffer.from(authString).toString('base64')}`, | ||
| 'content-type': APPLICATION_X_WWW_FORM_URLENCODED, | ||
| dpop: dpopHeader, | ||
| }, | ||
| body: 'grant_type=client_credentials&scope=webid', | ||
| }); | ||
| expect(res.status).toBe(200); | ||
| const json = await res.json(); | ||
| ({ access_token: accessToken } = json); | ||
| expect(typeof accessToken).toBe('string'); | ||
| }); | ||
|
|
||
| it('can use the generated access token to do an authenticated call.', async(): Promise<void> => { | ||
| const authFetch = await buildAuthenticatedFetch(accessToken!, { dpopKey }); | ||
| let res = await fetch(container); | ||
| expect(res.status).toBe(401); | ||
| res = await authFetch(container); | ||
| expect(res.status).toBe(200); | ||
| }); | ||
| }); | ||
|
|
||
| describe('setup', (): void => { | ||
| it('should contain the required configuration keys.', async(): Promise<void> => { | ||
| const res = await fetch(`${baseUrl}.well-known/openid-configuration`); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,6 @@ | |
| "css:config/http/server-factory/http.json", | ||
| "css:config/http/static/default.json", | ||
| "css:config/identity/access/public.json", | ||
| "css:config/identity/handler/default.json", | ||
| "css:config/identity/oidc/default.json", | ||
| "css:config/identity/ownership/token.json", | ||
|
|
@@ -39,7 +38,15 @@ | |
| "record": [ | ||
| { | ||
| "RecordObject:_record_key": "app", | ||
| "RecordObject:_record_value": { | ||
| "@id": "urn:solid-server:default:App" | ||
| } | ||
| }, | ||
| { | ||
| "RecordObject:_record_key": "store", | ||
| "RecordObject:_record_value": { | ||
| "@id": "urn:solid-server:default:ResourceStore_Backend" | ||
| } | ||
| } | ||
| ] | ||
| }, | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without that change
ClientIdAdapterFactorywas 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 settingtoken_endpoint_auth_methodtononeand complaining that it wasn't allowed and it didn't pick credentials passed viaAuthorizationhearder.I think it doesn't make sense to use
ClientIDAdapterat all whenClientCredentialsAdapterhandles 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.