-
Notifications
You must be signed in to change notification settings - Fork 36
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
WIP: Improve parser for WAC Allow header #1326
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,186 @@ | ||
/** | ||
* Copyright 2021 Inrupt Inc. | ||
* | ||
* Permission is hereby granted, free of charge, to any person obtaining a copy | ||
* of this software and associated documentation files (the "Software"), to deal in | ||
* the Software without restriction, including without limitation the rights to use, | ||
* copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the | ||
* Software, and to permit persons to whom the Software is furnished to do so, | ||
* subject to the following conditions: | ||
* | ||
* The above copyright notice and this permission notice shall be included in | ||
* all copies or substantial portions of the Software. | ||
* | ||
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, | ||
* INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A | ||
* PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT | ||
* HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION | ||
* OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE | ||
* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. | ||
*/ | ||
|
||
import { describe, it, expect } from "@jest/globals"; | ||
import { parseWacAllowHeader } from "./wacAllow.internal"; | ||
|
||
const noAccess = { | ||
read: false, | ||
append: false, | ||
write: false, | ||
control: false, | ||
}; | ||
|
||
const noPermissions = { | ||
user: noAccess, | ||
public: noAccess, | ||
}; | ||
|
||
describe("parseWacHeader", () => { | ||
it("should parse an empty header as no permissions", () => { | ||
const result = parseWacAllowHeader(""); | ||
|
||
expect(result).toHaveProperty("user"); | ||
expect(result).toHaveProperty("public"); | ||
|
||
expect(result).toMatchObject(noPermissions); | ||
}); | ||
|
||
it("should parse a single scope", () => { | ||
const result = parseWacAllowHeader('public="read"'); | ||
|
||
expect(result).toHaveProperty("user"); | ||
expect(result).toHaveProperty("public"); | ||
|
||
expect(result.user).toMatchObject(noAccess); | ||
|
||
expect(result.public).toMatchObject({ | ||
...noAccess, | ||
read: true, | ||
}); | ||
}); | ||
|
||
it("should parse multiple scopes", () => { | ||
const result = parseWacAllowHeader('user="write", public="read"'); | ||
|
||
expect(result).toHaveProperty("user"); | ||
expect(result).toHaveProperty("public"); | ||
|
||
expect(result.user).toMatchObject({ | ||
read: false, | ||
append: true, // note: write implies append | ||
write: true, | ||
control: false, | ||
}); | ||
|
||
expect(result.public).toMatchObject({ | ||
read: true, | ||
append: false, | ||
write: false, | ||
control: false, | ||
}); | ||
}); | ||
|
||
// This is subject to change, see: | ||
// https://github.com/solid/web-access-control-spec/issues/82 | ||
it("should handle parsing unknown access modes", () => { | ||
const result = parseWacAllowHeader( | ||
'public="read destroy",friends="love read"' | ||
); | ||
|
||
expect(result).toHaveProperty("user"); | ||
expect(result).toHaveProperty("public"); | ||
expect(result).not.toHaveProperty("friends"); | ||
|
||
expect(result.user).toMatchObject(noAccess); | ||
|
||
expect(result.public).toMatchObject({ | ||
read: true, | ||
append: false, | ||
write: false, | ||
control: false, | ||
}); | ||
}); | ||
|
||
it("should handle the append permission without write", () => { | ||
const result = parseWacAllowHeader('user="append read"'); | ||
|
||
expect(result).toHaveProperty("user"); | ||
expect(result).toHaveProperty("public"); | ||
|
||
expect(result.user).toMatchObject({ | ||
read: true, | ||
append: true, | ||
write: false, | ||
control: false, | ||
}); | ||
|
||
expect(result.public).toMatchObject(noAccess); | ||
}); | ||
|
||
it("should handle whitespace in header values", () => { | ||
const result = parseWacAllowHeader(' user = " append " '); | ||
|
||
expect(result).toHaveProperty("user"); | ||
expect(result).toHaveProperty("public"); | ||
|
||
// expects only append permission: | ||
expect(result.user).toMatchObject({ | ||
read: false, | ||
append: true, | ||
write: false, | ||
control: false, | ||
}); | ||
|
||
expect(result.public).toMatchObject(noAccess); | ||
}); | ||
|
||
describe("parsing of malformed headers", () => { | ||
it("missing quotes should result in no permissions", () => { | ||
const result = parseWacAllowHeader("user=write,public=read"); | ||
|
||
expect(result).toHaveProperty("user"); | ||
expect(result).toHaveProperty("public"); | ||
|
||
expect(result).toMatchObject(noPermissions); | ||
}); | ||
|
||
it("missing quotes on one group should not affect the other", () => { | ||
const result = parseWacAllowHeader('user=write,public="read"'); | ||
|
||
expect(result).toHaveProperty("user"); | ||
expect(result).toHaveProperty("public"); | ||
|
||
expect(result.user).toMatchObject(noAccess); | ||
expect(result.public).toMatchObject({ | ||
...noAccess, | ||
read: true, | ||
}); | ||
}); | ||
|
||
it("mismatched quotes should result in no permissions", () => { | ||
const result = parseWacAllowHeader('user="write,public=read"'); | ||
|
||
expect(result).toHaveProperty("user"); | ||
expect(result).toHaveProperty("public"); | ||
|
||
expect(result).toMatchObject(noPermissions); | ||
}); | ||
|
||
it("missing scope should result in no permissions", () => { | ||
const result = parseWacAllowHeader('="write read"'); | ||
|
||
expect(result).toHaveProperty("user"); | ||
expect(result).toHaveProperty("public"); | ||
|
||
expect(result).toMatchObject(noPermissions); | ||
}); | ||
|
||
it("missing/empty statement should result in no permissions", () => { | ||
const result = parseWacAllowHeader('public=" "'); | ||
|
||
expect(result).toHaveProperty("user"); | ||
expect(result).toHaveProperty("public"); | ||
|
||
expect(result).toMatchObject(noPermissions); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
/** | ||
* Copyright 2021 Inrupt Inc. | ||
* | ||
* Permission is hereby granted, free of charge, to any person obtaining a copy | ||
* of this software and associated documentation files (the "Software"), to deal in | ||
* the Software without restriction, including without limitation the rights to use, | ||
* copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the | ||
* Software, and to permit persons to whom the Software is furnished to do so, | ||
* subject to the following conditions: | ||
* | ||
* The above copyright notice and this permission notice shall be included in | ||
* all copies or substantial portions of the Software. | ||
* | ||
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, | ||
* INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A | ||
* PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT | ||
* HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION | ||
* OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE | ||
* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. | ||
*/ | ||
|
||
// TODO: Should we move this type internally, as the Access in this file is not strictly the same as ACL Access | ||
import { Access } from "../acl/acl"; | ||
|
||
export type internal_AccessPermissions = { | ||
user: Access; | ||
public: Access; | ||
}; | ||
|
||
interface internal_parsedWacHeader { | ||
[group: string]: string[]; | ||
} | ||
|
||
function internal_parseWacHeader(header: string): internal_parsedWacHeader { | ||
const rawEntries = header.split(",").map((rawEntry) => rawEntry.split("=")); | ||
|
||
const entries: internal_parsedWacHeader = {}; | ||
|
||
for (const rawEntry of rawEntries) { | ||
if (rawEntry.length !== 2) { | ||
continue; | ||
} | ||
|
||
const scope = rawEntry[0].trim(); | ||
const statement = rawEntry[1].trim(); | ||
|
||
if (!scope || !statement) { | ||
continue; | ||
} | ||
|
||
if ( | ||
statement.length === 2 || | ||
statement.charAt(0) !== '"' || | ||
statement.charAt(statement.length - 1) !== '"' | ||
) { | ||
continue; | ||
} | ||
|
||
const accessModes = statement.substring(1, statement.length - 1).trim(); | ||
|
||
if (accessModes.length === 0) { | ||
continue; | ||
} | ||
|
||
entries[scope] = accessModes.split(" "); | ||
} | ||
|
||
return entries; | ||
} | ||
|
||
function internal_getWacPermissions( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure this is named correctly / strictly belongs here; perhaps There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me, we can always move it if it becomes relevant elsewhere. |
||
parsed: internal_parsedWacHeader, | ||
scope: string | ||
): Access { | ||
const permissions = parsed[scope]; | ||
|
||
if (!permissions || !Array.isArray(permissions)) { | ||
return { | ||
read: false, | ||
append: false, | ||
write: false, | ||
control: false, | ||
}; | ||
} | ||
|
||
return { | ||
read: permissions.includes("read"), | ||
append: permissions.includes("append") || permissions.includes("write"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the same logic as, but simpler than: https://github.com/inrupt/solid-client-js/blob/main/src/resource/resource.internal.ts#L87-L100 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the way https://github.com/inrupt/solid-client-js/blob/main/src/resource/resource.internal.ts#L87-L100 is written was motivated by type definitions, if I recall correctly the TS compiler was complaining if the |
||
write: permissions.includes("write"), | ||
control: permissions.includes("control"), | ||
}; | ||
} | ||
|
||
/** | ||
* Parse a WAC-Allow header into user and public access booleans. | ||
* | ||
* @param wacAllowHeader A WAC-Allow header in the format `user="read append write control",public="read"` | ||
* @see https://github.com/solid/solid-spec/blob/cb1373a369398d561b909009bd0e5a8c3fec953b/api-rest.md#wac-allow-headers | ||
*/ | ||
export function parseWacAllowHeader( | ||
wacAllowHeader: string | ||
): internal_AccessPermissions { | ||
const parsed = internal_parseWacHeader(wacAllowHeader); | ||
|
||
return { | ||
user: internal_getWacPermissions(parsed, "user"), | ||
public: internal_getWacPermissions(parsed, "public"), | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NSeydoux @VirginiaBalseiro what're your thoughts on this one? I think parsing WAC Allow shouldn't be re-using data structures from ACL, as they are different things (the types just happen to currently be compatible)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it kind of makes sense that the
Access
definition defined in ACL is used for the WAC-Allow header, given that the WAC-Allow header was initially described in the ACL spec. We have anotherAccess
type definition from theaccess/universal
submodule for universal (i.e. for both ACL-controlled and ACP-controlled Pods), which is somewhat different (because ACP has a different notion of Control than ACL), but could maybe used here, since the WAC-Allow header is now present in both ACL and ACP specs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to rename
Access
in./acl/acl
toAclAccess
(we would need to export it still asAccess
but mark that as a deprecated name (that'd give interface/type naming consistency withinAcl
Then have
wacAllow
use ainternal_Access
type that's compatible? There's also aWacAccess
in https://github.com/inrupt/solid-client-js/blob/c8da4e5b0a7baa20ff04558b7e06d1a42e8df3ae/src/access/wac.ts — I'm not sure what this is exactly, and it's undocumented in the currently released SDK version, as far as I can tell.Perhaps we move the WacAccess up to a
./wac-allow
directory, and moveWacAccess
and the parser / transform there?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Access interface (there is no reason for it to be a type considering its declaration) should in my opinion:
/src/type/
It would be particularly convenient to have one place where all the standard interfaces are defined.
We really need to remove all the aliasing going on, it is exactly the opposite of what interfaces are for. You don't want 20 names for the same thing, you want 1 name for a specific shape and as few of them as required.
As a sidenote, the ACP specification will adopt the standard Solid access modes shortly, that is, use the
acl:Control
mode just like WAC. It is a design flaw that it currently invents another mechanism. The operations entailed by Access Modes are defined at the Solid protocol level.