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

Default pod template sets rights on / instead of index.html #1065

Merged
merged 2 commits into from
Jan 28, 2019

Conversation

RubenVerborgh
Copy link
Contributor

Partial fix for #1063.

a acl:Authorization;
acl:agentClass foaf:Agent;
acl:accessTo </>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we state that index.html is publicly readable as well, since we're deleting index.html.acl?

If we don't, people won't be able to access /index.html, but they would be able to access /. That might be a use case that's not that important anyway though, so might not be necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, was planning to say we could change line 8 to acl:accessTo </>, </index.html>, but that doesn't work either...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would refrain from including anything /index/ in the ACL files. As far as the user is concerned, these don't really (need to) exist. They are a convention; we could start naming them default.html in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's part of the broader point in solid/web-access-control-spec#36.

Copy link
Contributor

@megoth megoth left a comment

Choose a reason for hiding this comment

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

This should be sound changes. Will need a fix to make old accounts serve the homepage as is expected from earlier version, as @RubenVerborgh have said in #1063 (comment)

@kjetilk
Copy link
Member

kjetilk commented Jan 25, 2019

So, to add to the confusion, I just did

kjetil@robin:~$ dog -k https://kjetiltest.inrupt.net/
HTTP/1.1 401 Unauthorized

(where dog is an alias analogous to cat but with network :-) )

So, it appears to me that the root is not public readable by default in the current code...?

@RubenVerborgh
Copy link
Contributor Author

RubenVerborgh commented Jan 25, 2019

So, it appears to me that the root is not public readable by default in the current code...?

Indeed, this PR should fix that for new pods.

Also, you should really publish your dotfiles on GitHub, so we can all use those funny aliases 😉

@kjetilk
Copy link
Member

kjetilk commented Jan 25, 2019

So, it appears to me that the root is not public readable by default in the current code...?

Indeed, this PR should fix that for new pods.

What I mean is that it is not public readable for 4.4... Which would imply no regression. But I'm probably just confused.

Also, you should really publish your dotfiles on GitHub, so we can all use those funny aliases wink

Hehe, there's not a lot. That is, I have a /site/ dir that are mounted or rsynced to all my boxes. Some stuff in there, but the aliases I'm actually using are these:

alias ..='cd ../'
alias ...='cd ../../'
alias ....='cd ../../../'
alias .....='cd ../../../../'
alias deluntrack='for f in $(git st --porcelain | grep ?? | cut -b4-) ; do rm -r $f; done'
alias dir='ls -al | less -X -E'
alias dog='curl -D - '
alias lr='ls -ltr'

Perhaps one of these days, I might put them somewhere. :-) Meanwhile, apologies for the OT

@RubenVerborgh
Copy link
Contributor Author

RubenVerborgh commented Jan 25, 2019 via email

@leinue
Copy link

leinue commented Jan 26, 2019

I encountered this problem after upgrading NSS5, looking forward to solving.

@kjetilk
Copy link
Member

kjetilk commented Jan 26, 2019

No regression: this is a consequence of acl-check, which isn’t in 4.4, right?

It wouldn't be a direct consequence of acl-check, it might be a consequence of rewriting the tree traversal code, or the introduction of ResourceMapper, but, the initial observation that @megoth made was that:

How it works on v4: as a visitor I have read access to both https://megoth.solid.community/ and https://megoth.solid.community/index.html

This is what I fail to reproduce, both on inrupt.net and solid.community, both of which runs 4.x series server.

I haven't got read access to https://kjetiltest.solid.community/ nor to https://kjetiltest.inrupt.net/ so with these newly created accounts, the behaviour of 4.x is identical to 5.x-beta.5, as far as I can tell.

Now, if that makes sense is another matter, I tend to agree that / (but only that, i.e. no acl:default predicate) should be readable to public by default, but there may be design decisions behind that it isn't.

So, there are two questions:

  1. If @megoth 's observed behaviour is representative, or a result of having changed the ACL.
  2. If we should make / public readable by default.

But I may just be confused.

@kjetilk kjetilk self-requested a review January 26, 2019 01:14
Copy link
Member

@kjetilk kjetilk left a comment

Choose a reason for hiding this comment

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

Just leaving a request changes, to allow us(/me) to grok what the issue really is.

Copy link
Member

@kjetilk kjetilk left a comment

Choose a reason for hiding this comment

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

Yes, as expressed in #1063 , I now think this is the main solution.

@kjetilk kjetilk merged commit ec068f3 into release/v5.0.0 Jan 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants