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

Add support of multi oidcIssuer relations per one user's profile #30

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
2 changes: 1 addition & 1 deletion src/oidc-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ class OidcManager {
}

// Otherwise, verify that issuer is the preferred OIDC provider for the web id
return discoverProviderFor(webId)
return discoverProviderFor(webId, issuer)
.then(preferredProvider => {
if (preferredProvider === issuer) { // everything checks out
return webId
Expand Down
66 changes: 54 additions & 12 deletions src/preferred-provider.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,20 +61,48 @@ function providerExists (uri) {
* given Web ID, extracted from Link rel header or profile body. If no
* provider URI was found, reject with an error.
*/
function discoverProviderFor (webId) {
function discoverProviderFor (webId, issuer) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the method functionality has changed from discovering to validating? We probably need a method like isValidProvider then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will update the code.
I tried to create fix with a minimal changes in the code.

return discoverFromHeaders(webId)

.then(providerFromHeaders => providerFromHeaders || discoverFromProfile(webId))
.then(providerFromHeaders => providerFromHeaders || discoverAllFromProfile(webId))

.then(providerUri => {
// drop the path (provider origin only)
if (providerUri) {
providerUri = (new URL(providerUri)).origin
if (Array.isArray(providerUri)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not good; should always be an array.

Copy link
Contributor

@megoth megoth Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could force it into an array with [].concat(providerUri) (if there are possible cases where it's not - which I guess shouldn't be in this case; nevertheless, it's a nice "hack" to know of)

let list = providerUri
let lastErr = null

for (let i = 0; i < list.length; i++) {
lastErr = null
providerUri = list[i]
if (providerUri) {
providerUri = (new URL(providerUri)).origin
}

try {
validateProviderUri(providerUri, webId) // Throw an error if empty or invalid
} catch (err) {
lastErr = err
}

if (lastErr === null && ((issuer && providerUri === issuer) || !issuer)) {
return providerUri
}
}
if (lastErr) {
throw lastErr
} else {
validateProviderUri(null, webId) // Throw an error if empty or invalid
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above logic is very messy.

// drop the path (provider origin only)
if (providerUri) {
providerUri = (new URL(providerUri)).origin
}

validateProviderUri(providerUri, webId) // Throw an error if empty or invalid

return providerUri
}

validateProviderUri(providerUri, webId) // Throw an error if empty or invalid

return providerUri
})
}

Expand All @@ -94,16 +122,30 @@ function discoverFromHeaders (webId) {
})
}

function discoverFromProfile (webId) {
function discoverAllFromProfile (webId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should then also become validateFromProfile and accept a second issuer parameter. Will eliminate most of the messy logic above.

const store = rdf.graph()

const fetcher = rdf.fetcher(store)

return fetcher.load(webId, { force: true })
.then(response => {
if (!response.ok) {
let error = new Error(`Could not reach Web ID ${webId} to discover provider`)
error.statusCode = 400
throw error
}

let providerTerm = rdf.namedNode('http://www.w3.org/ns/solid/terms#oidcIssuer')
let providerUri = store.anyValue(rdf.namedNode(webId), providerTerm)
return providerUri
let idp = store.each(rdf.namedNode(webId), providerTerm, undefined)
let list = []

for (let i = 0; i < idp.length; i++) {
if (idp[i].uri) {
list.push(idp[i].uri)
}
}

return list
}, err => {
let error = new Error(`Could not reach Web ID ${webId} to discover provider`)
error.cause = err
Expand Down
20 changes: 20 additions & 0 deletions test/resources/sample-webid-profile1.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
module.exports = `
@prefix solid: <http://www.w3.org/ns/solid/terms#>.
@prefix foaf: <http://xmlns.com/foaf/0.1/>.
@prefix pim: <http://www.w3.org/ns/pim/space#>.
@prefix schema: <http://schema.org/>.
@prefix ldp: <http://www.w3.org/ns/ldp#>.

<>
a foaf:PersonalProfileDocument ;
foaf:primaryTopic <#me> .

<#me>
a schema:Person ;

solid:account </> ; # link to the account uri
pim:storage </> ; # root storage

solid:oidcIssuer <https://provider2.com> ;
solid:oidcIssuer <https://provider.com> .
`
17 changes: 17 additions & 0 deletions test/unit/preferred-provider-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,23 @@ describe('preferred-provider.js', () => {
})
})

it('should extract and validate the provider uri from the webid profile with multi oidcIssuer', () => {
nock(serverUri)
.options('/')
.reply(204, 'No content')

nock(serverUri)
.get('/')
.reply(200, sampleProfileSrc, {
'Content-Type': 'text/turtle'
})

return provider.discoverProviderFor(webId, 'https://provider.com')
.then(providerUri => {
expect(providerUri).to.equal('https://provider.com')
})
})

it('should throw an error if webid is reachable but no provider uri found', done => {
nock(serverUri)
.options('/')
Expand Down