Skip to content

Fixed issuer path trimming #58

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 24, 2020
Merged

Fixed issuer path trimming #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions src/preferred-provider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,6 @@ function discoverProviderFor (webId) {
.then(providerFromHeaders => providerFromHeaders || discoverFromProfile(webId))

.then(providerUri => {
// drop the path (provider origin only)
Copy link
Contributor

@pmcb55 pmcb55 Nov 24, 2020

Choose a reason for hiding this comment

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

Is this really appropriate (I'm just checking here, I honestly have no idea what the 'correct behaviour' should be)? i.e. I know we don't want to strip off any trailing slashes from the origin (i.e. if the URI is https://provider.com/, then we do not want https://provider.com), but do we also want to keep the entire path if there is one (i.e. if the URI was https://provider.com/some/path, do we really want https://provider.com/some/path, or do we actually need https://provider.com/)...? (Regardless, it seems any such logic belongs in the validateProviderUri() function, if indeed we do need any logic to mess around with the providerUri value.)

Copy link
Member

@michielbdejong michielbdejong Nov 24, 2020

Choose a reason for hiding this comment

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

Why do we not want to strip off a trailing slash? I can see that the spec doesn't forbid its presence, but what is the function of keeping it?

Copy link
Contributor Author

@jaxoncreed jaxoncreed Nov 24, 2020

Choose a reason for hiding this comment

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

@pmcb55

if the URI was https://provider.com/some/path, do we really want https://provider.com/some/path, or do we actually need https://provider.com/)...?

Yes, we would want it to be https://provider.com/some/path. There is no reason someone would put an extra path in their WebId. We do not want to automagically equate an issuer at https://provider.com/some/path with one at https://provider.com/ as they could be two separate issuers. So I don't think any additional logic is needed.

@michielbdejong

what is the function of keeping it?

This particular section is checking if ISSUER_LISTED_IN_THE_WEBID === ISSUER_IN_THE_TOKEN. Right now, for ESS both the issuer in the webId and the issuer in the token are https://example.com/, but the statement I removed changes the issuer.

if (providerUri) {
providerUri = (new URL(providerUri)).origin
}

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

return providerUri
Expand Down
4 changes: 2 additions & 2 deletions test/unit/preferred-provider-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ describe('preferred-provider.js', () => {
})
})

it('should drop the path from extracted provider uri', () => {
it('should not drop the path from extracted provider uri', () => {
nock(serverUri)
.options('/')
.reply(204, 'No content', {
Expand All @@ -43,7 +43,7 @@ describe('preferred-provider.js', () => {

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

Expand Down