Added special ACL handling for root #1067
Conversation
megoth
requested review from
kjetilk,
rubensworks,
RubenVerborgh and
dmitrizagidulin
There was a problem hiding this comment.
I don't quite like the way this is going. I think that the ACL of the representation shouldn't be used for the request URL, since the client is requesting a specific resource and has no idea of what the server does internally to return a representation.
Now, it is confusing that there would in practice be an ACL for the request URI that may be different from the ACL that applies to the representation, but this is a problem for all containers, not just for root. We might want to address that somehow, but now, I think we should treat them as two different resources, since they are identified by two different URLs.
So, we can revisit at some point, but I think we should not apply this now.
We agree, but have you seen #1063 (comment)? |
|
@RubenVerborgh Yeah, but I commented on that too, please see my last comment there, where I say that I just want to accomodate existing PODs by supplying a script, not to make any changes to accomodate for it in the code. So I guess we disagree based on that comment. :-) Lets discuss over in #1063 |
|
I'm OK with it now, but I have to admit that I find it harder to review when it contains that extensive refactorings. Perhaps we should do refactoring in separate PRs in the future? Nice if we can get another review too. |
I could copy a lot of code instead of refactoring it, yes, but please lets do a separate refactoring at once after, ok? If we don't, we'll get (even more) unmaintainable code... |
No, I like the fact that you're refactoring, we should do that. I just wonder if we could do it differently. Perhaps there is something around that could tell us some practices around that. What if you first branch, refactor the stuff you need to refactor, then branch off of that branch again to implement the new functionality. PRs could be made for both, so people can review them apart or together. Or perhaps that would add too much overhead? We should discuss at some point. |
Ah, so have a separate branch with the refactorings... yeah, I could do that. Not something I have done in other projects, but than again, not so much work regarding branches and stuff before either =P I'll do a bit of research, and see what good practices are out there. |
I don't have that much time to do research, but I propose using the pattern |
To not break old PODs that have a root ACL that doesn't give READ access to root - will check for the ACL for the representation that is served back for root
To not break old PODs that have a root ACL that doesn't give READ access to root - will check for the ACL for the representation that is served back for root Hopefully making things a bit easier to read Added comment about the hack
The hack will only work when it's a HTML file that is returned as the representation for the root resource
megoth
force-pushed
the
bug/root-index-special-handling
branch
from
6723bd0 to
f71e582
Compare
This is a hack that should fix #1063.
In order to not break old PODs that have a root ACL that doesn't give READ access to root, we want to also check the ACL for the representation that the server intends to return for root.
I'm uncertain of the tests I changed in
test/integration/authentication-oidc-test.js, so review those with extra scrutiny.I've rewritten the creation of ACLChecker into createFromLDPAndRequest, the same as I did in #1060. This might not be a very good solution, but it works, and I propose we rewrite it as a separate PR some time in the future when we have thought of a better pattern.