Skip to content

Conversation

@RubenVerborgh
Copy link
Contributor

@RubenVerborgh RubenVerborgh commented Aug 17, 2017

Implements #246 with WAC-Allow: user="read write append",public="read".

@csarven
Copy link
Member

csarven commented Aug 17, 2017

Good stuff!

Would "WAC-Mode" be preferable just to keep it closer to *nix's mode bits?

@RubenVerborgh
Copy link
Contributor Author

@csarven The header name was chosen in a discussion with @timbl and @dmitrizagidulin yesterday (see #246). I don't mind the naming either way (although I still think "WAC" is cryptic).

@dmitrizagidulin
Copy link
Contributor

I just realized we want to do WAC-Allow, minus the ed on the end. (To match the HTTP Allow: header). Sorry bout that :)

@RubenVerborgh RubenVerborgh force-pushed the feature/permissions-in-header branch 3 times, most recently from e7e7911 to b792b9a Compare August 17, 2017 21:37
Copy link
Contributor

@timbl timbl left a comment

Choose a reason for hiding this comment

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

On the whiteboard we had spaces or commas to separate the modes, to mimic other headers. In this code we have semicolons... was that a conscious decision?

@RubenVerborgh
Copy link
Contributor Author

Which of these would you prefer? I don't mind either way; I was following what @dmitrizagidulin wrote down of our discussion in #246.

@csarven
Copy link
Member

csarven commented Aug 18, 2017

To stick close to convention (like in Web Linking for relation types), probably best to use space as a delimiter. Plus, it is easier to read.

@RubenVerborgh
Copy link
Contributor Author

Makes sense! Are the quotes needed?

@csarven
Copy link
Member

csarven commented Aug 18, 2017

I think the quotes might make it easier for consumers to tokenise. It can also help to escape characters or have specialised modes or something in the future.

@RubenVerborgh
Copy link
Contributor Author

Okay, mandatory quotes it is.

@RubenVerborgh RubenVerborgh force-pushed the feature/permissions-in-header branch from b792b9a to 250e513 Compare August 18, 2017 13:28
@RubenVerborgh
Copy link
Contributor Author

250e513 brings spaces.

@RubenVerborgh
Copy link
Contributor Author

Currently on hold because of #552.

@RubenVerborgh RubenVerborgh force-pushed the feature/permissions-in-header branch from 250e513 to 4b6a381 Compare August 18, 2017 18:58
@RubenVerborgh
Copy link
Contributor Author

#552 has been merged, so this one is ready for review and merge.

Copy link
Contributor

@dmitrizagidulin dmitrizagidulin left a comment

Choose a reason for hiding this comment

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

This has been a much-requested feature! Nicely done!

@dmitrizagidulin dmitrizagidulin merged commit 2b8f18b into dz_oidc Aug 18, 2017
@dmitrizagidulin dmitrizagidulin deleted the feature/permissions-in-header branch August 18, 2017 19:09
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.

5 participants