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

fix: Add webid claim to webid scope #1040

Merged
merged 1 commit into from Nov 8, 2021
Merged

fix: Add webid claim to webid scope #1040

merged 1 commit into from Nov 8, 2021

Conversation

joachimvh
Copy link
Member

Closes #1038.

I don't think this breaks anything for older versions but there might be a case I'm not aware of.

@joachimvh
Copy link
Member Author

I don't think this breaks anything for older versions but there might be a case I'm not aware of.

It does break some things. Was testing Penny and logging in no longer worked with this change:

Error: Cannot extract WebID from ID token: the ID token returned by [http://localhost:3000/] has no 'webid' claim, nor an IRI-like 'sub' claim: [f3ad3af49e58b5a536e430d2d8edfc1051ff0cec2810b62fff040c6f66a231e8]. Attempting to construct a URL from the 'sub' claim threw: TypeError: URL constructor: f3ad3af49e58b5a536e430d2d8edfc1051ff0cec2810b62fff040c6f66a231e8 is not a valid URL.

It makes sense that Penny doesn't get the webid claim anymore since it's probably not requesting the new webid scope. Our sub field is also supposed to contain the webid though I think (even though not requested by the spec) so will have to investigate.

@joachimvh
Copy link
Member Author

Our sub field is also supposed to contain the webid though I think (even though not requested by the spec) so will have to investigate.

See #1041

@RubenVerborgh
Copy link
Member

Is penny just using an older version of the auth client perhaps, @Vinnl?

"Sets all the relevant oidc parameters.",
"webid claim is in openid scope until an official scope has been decided: https://github.com/solid/authentication-panel/issues/86"
],
"comment": "Sets all the relevant oidc parameters.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"comment": "Sets all the relevant oidc parameters.",
"comment": "Sets all the relevant Solid-OIDC parameters.",

@Vinnl
Copy link

Vinnl commented Nov 2, 2021

@RubenVerborgh I don't know what "old" is, but it currently has defined the version range ^1.8.0. On penny.vincenttunru.com that resolves to 1.8.0, release in April 2021.

Did Solid/CSS have breaking authn changes again since April?

Copy link
Contributor

@acoburn acoburn left a comment

Choose a reason for hiding this comment

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

LGTM

Note that the line at https://github.com/solid/community-server/pull/1040/files#diff-9732c16d61c118aef1a2a2594763b5731a7fe9fdc60ae2b351cc6369a362205bR24 (solid_oidc_supported) is no longer required by the Solid-OIDC draft. There is no harm in leaving it in place, but discovery now happens via the mechanism added here (webid scope)

Also a note in passing about https://github.com/solid/community-server/pull/1040/files#diff-9732c16d61c118aef1a2a2594763b5731a7fe9fdc60ae2b351cc6369a362205bR29 (dPoP ... draft-01) the current DPoP draft is at version 04

@RubenVerborgh
Copy link
Member

Did Solid/CSS have breaking authn changes again since April?

The authn spec is still evolving; we follow it closely.

I don't know what "old" is, but it currently has defined the version range ^1.8.0.

That seems old indeed; on normal npm installs, the latest version would be chosen with that range. But given that Penny is precompiled with the package-lock versions, it seems to be an old version indeed.

@RubenVerborgh
Copy link
Member

Also a note in passing about https://github.com/solid/community-server/pull/1040/files#diff-9732c16d61c118aef1a2a2594763b5731a7fe9fdc60ae2b351cc6369a362205bR29 (dPoP ... draft-01) the current DPoP draft is at version 04

@joachimvh Let's add that clearly as a comment in our config.

@Vinnl
Copy link

Vinnl commented Nov 2, 2021

The authn spec is still evolving; we follow it closely.

Do you know if there were breaking changes since April, and which version of SCAB is compatible with those changes? I can push an update, but the newer the version the more work it'll be, so if I can narrow it down to a smaller upgrade I can do it more quickly :)

@RubenVerborgh
Copy link
Member

Do you know if there were breaking changes since April

I do not; perhaps @NSeydoux does.

I can push an update, but the newer the version the more work it'll be

It's a semver.minor, so should not be any work apart from updating package-lock.
If not, we got semver wrong.

@joachimvh joachimvh changed the base branch from main to versions/3.0.0 November 8, 2021 10:28
@joachimvh joachimvh added the semver.major Requires a major version bump label Nov 8, 2021
@joachimvh joachimvh merged commit fc60b5c into versions/3.0.0 Nov 8, 2021
@joachimvh joachimvh deleted the fix/webid branch November 8, 2021 10:39
@NSeydoux
Copy link

I just merged inrupt/solid-client-authn-js@e9e89f7 to add the webid scope to solid-client-authn. I'll cut a release soon.

Do you know if there were breaking changes since April

There has been some changes in the spec, not all of which is reflected in solid-client-authn yet. For instance, the OIDC issuer issuing an Access Token will no longer be the only authentication pattern, and resource servers (RS) may elect on a per-case basis to run their own authorization server (AS). This way, the range of action of a given access token is reduced (it's only useful to the RS associated to a given AS. Additional information is available in the authN panel minutes, and I'm happy to answer any additional question on the matter :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver.major Requires a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add webid to supported scopes?
5 participants