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 note for WebID authenticated fetch #1709

Merged
merged 2 commits into from Jan 20, 2023
Merged

Conversation

NSeydoux
Copy link
Contributor

WebID must be public resources, and may not be Solid resources. As a result, by default they should be fetched unauthenticated. This commit simply adds a note describing this already implemented behavior.

@NSeydoux NSeydoux requested a review from a team as a code owner September 13, 2022 09:18
@vercel
Copy link

vercel bot commented Sep 13, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
solid-client-js ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 20, 2023 at 3:20PM (UTC)

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 1adc390:

Sandbox Source
solid-client-sandbox Configuration

@NSeydoux NSeydoux temporarily deployed to Inrupt Dev-Next September 13, 2022 09:18 Inactive
@NSeydoux NSeydoux temporarily deployed to Inrupt Dev-Next September 13, 2022 09:18 Inactive
@NSeydoux NSeydoux temporarily deployed to Inrupt Dev-Next September 13, 2022 09:18 Inactive
@vercel vercel bot temporarily deployed to Preview September 13, 2022 09:18 Inactive
@@ -76,7 +76,9 @@ export function getAltProfileUrlAllFrom(
* A WebID Profile may be any RDF resource on the Web, it doesn't have
* to be a Solid resource. That is why, in order to expose a Solid-enabled part
* of their profile, some WebID profiles link to a Profile Resource, which may
* be a Solid resource.
* be a Solid resource. A WebID resource should be public, so `getProfileAll` will
Copy link
Contributor

Choose a reason for hiding this comment

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

So ... since we state "to expose a Solid-enabled part ..." above, should "a Profile Resource, which may be a Solid resource" be just "a Profile Resource, which is ..."? Or even "an Extended Profile, which is a Solid Resource"?

And, with "WebID resource should be public" -- is it a should be or is it a must? If a must, I'd just say "A WebID profile is publicly readable"

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ... also, in case your "should be public" stands -- such that, some people make their profiles not publicly readable (mistakenly or not), just wondering, for the WebID profile part of getProfileAll, do we first do an unauthed fetch, and then, if that fails, we do an authe'd fetch?

Copy link
Contributor

Choose a reason for hiding this comment

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

"Do we first do a...", yes, that's the implementation: https://github.com/inrupt/solid-client-js/pull/1709/files#diff-df051b184aa88ad98045d31b7def5698e9b5c75b17fd85b15bc8a8790cf82798R122

keep in mind this isn't strictly specified by a spec yet.

Copy link
Contributor

@kay-kim kay-kim left a comment

Choose a reason for hiding this comment

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

lgtm - left some comments.

@ThisIsMissEm
Copy link
Contributor

Why don't we just add a "getWebId" method?

@NSeydoux
Copy link
Contributor Author

We can also add a getWebID, but that's a separate issue I think, we'll still want to be able to get both the WebID and all available extended profiles in one call.

@ThisIsMissEm
Copy link
Contributor

@NSeydoux hmm, okay, yeah, agreed.

@NSeydoux NSeydoux temporarily deployed to Inrupt Dev-Next November 2, 2022 09:16 Inactive
@NSeydoux NSeydoux temporarily deployed to NSS November 2, 2022 09:16 Inactive
@NSeydoux NSeydoux temporarily deployed to Inrupt Dev-Next November 2, 2022 09:17 Inactive
@NSeydoux NSeydoux temporarily deployed to Inrupt Dev-Next November 2, 2022 09:17 Inactive
@NSeydoux NSeydoux temporarily deployed to Inrupt Dev-Next November 2, 2022 09:17 Inactive
@vercel vercel bot temporarily deployed to Preview November 2, 2022 09:17 Inactive
@@ -76,7 +76,9 @@ export function getAltProfileUrlAllFrom(
* A WebID Profile may be any RDF resource on the Web, it doesn't have
* to be a Solid resource. That is why, in order to expose a Solid-enabled part
* of their profile, some WebID profiles link to a Profile Resource, which may
* be a Solid resource.
* be a Solid resource. A WebID resource should be public, so `getProfileAll` will
Copy link
Contributor

Choose a reason for hiding this comment

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

"Do we first do a...", yes, that's the implementation: https://github.com/inrupt/solid-client-js/pull/1709/files#diff-df051b184aa88ad98045d31b7def5698e9b5c75b17fd85b15bc8a8790cf82798R122

keep in mind this isn't strictly specified by a spec yet.

@@ -76,7 +76,9 @@ export function getAltProfileUrlAllFrom(
* A WebID Profile may be any RDF resource on the Web, it doesn't have
* to be a Solid resource. That is why, in order to expose a Solid-enabled part
* of their profile, some WebID profiles link to a Profile Resource, which may
* be a Solid resource.
* be a Solid resource. A WebID resource should be public, so `getProfileAll` will
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* be a Solid resource. A WebID resource should be public, so `getProfileAll` will
* be a Solid resource. A WebID resource MUST be public, so `getProfileAll` will

I think this is what the spec says, but I might be wrong

Copy link
Contributor

Choose a reason for hiding this comment

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

Spec doesn't say "MUST" nor "should", in fact, it no longer says anything about WebID's other than they're resources — i.e., it reintroduced as of September 10th the ability for WebID's to require authentication to read, by way of omitting to specify that they must be publicly readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arguably, https://www.w3.org/2005/Incubator/webid/spec/identity/ can be interpreted to mandate the WebID to be public (that's the worst sentence to say about a specification), so unless the Solid-WebID spec adds additional constraints, it sounds like a fair assumption that WebID profiles are public documents.

@ThisIsMissEm
Copy link
Contributor

Relates to #1761

@NSeydoux NSeydoux temporarily deployed to Inrupt Dev-Next November 24, 2022 13:22 Inactive
@NSeydoux NSeydoux temporarily deployed to NSS November 24, 2022 13:22 Inactive
@vercel vercel bot temporarily deployed to Preview November 24, 2022 13:23 Inactive
@NSeydoux NSeydoux temporarily deployed to Inrupt Dev-Next November 24, 2022 13:23 Inactive
@NSeydoux NSeydoux temporarily deployed to Inrupt Dev-Next November 24, 2022 13:23 Inactive
@NSeydoux NSeydoux temporarily deployed to Inrupt Dev-Next November 24, 2022 13:23 Inactive
@ThisIsMissEm
Copy link
Contributor

Let's merge after #1765 is merged and the new environments roll out, that'll fix the CI issues here.

WebID must be public resources, and may not be Solid resources. As a result, by default they should be fetched unauthenticated. This commit simply adds a note describing this already implemented behaviour.
@ThisIsMissEm ThisIsMissEm temporarily deployed to ESS PodSpaces January 20, 2023 15:15 — with GitHub Actions Inactive
@ThisIsMissEm ThisIsMissEm temporarily deployed to ESS PodSpaces January 20, 2023 15:15 — with GitHub Actions Inactive
@ThisIsMissEm ThisIsMissEm temporarily deployed to ESS Dev-Next January 20, 2023 15:15 — with GitHub Actions Inactive
@ThisIsMissEm ThisIsMissEm temporarily deployed to ESS PodSpaces January 20, 2023 15:15 — with GitHub Actions Inactive
@ThisIsMissEm ThisIsMissEm temporarily deployed to NSS January 20, 2023 15:15 — with GitHub Actions Inactive
@ThisIsMissEm ThisIsMissEm temporarily deployed to ESS PodSpaces January 20, 2023 15:15 — with GitHub Actions Inactive
@ThisIsMissEm ThisIsMissEm temporarily deployed to NSS January 20, 2023 15:15 — with GitHub Actions Inactive
@ThisIsMissEm ThisIsMissEm temporarily deployed to ESS Dev-Next January 20, 2023 15:15 — with GitHub Actions Inactive
@ThisIsMissEm ThisIsMissEm merged commit 51e5ed2 into main Jan 20, 2023
@ThisIsMissEm ThisIsMissEm deleted the fix/get-profile-all-doc branch January 20, 2023 15:34
solid-akb pushed a commit that referenced this pull request Jan 25, 2023
* Add note for WebID authenticated fetch

WebID must be public resources, and may not be Solid resources. As a result, by default they should be fetched unauthenticated. This commit simply adds a note describing this already implemented behaviour.

* Mention Extended Profiles
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants