Block cloning from brew metadata

[5e-Cleric]

I have added an extra field to control if a brew should be able to be cloned.

• Disable navbar elements if cloning disabled
• Add UI to change cloning state
• Disable routes to clone
• Handle Source and Download in a react component, to check the user account
• Add error pages

[5e-Cleric]

This is no longer a small PR, i am sorry.

The new share page if the brew's cloning is allowed or if you are the author:

Show image

The error page if cloning is not allowed and you are not the author:

Show image

UI to change cloning state:

[ericscheid]

Possibly should use 401 UNAUTHORISED instead of 423 LOCKED. The brew is not locked, the user needs to authenticate with credentials that are allowed to clone this brew. (Also not 403 FORBIDDEN, which is for when the particular action is not allowed at all, for any user.)

The HTTP 401 Unauthorized client error response status code indicates that a request was not successful because it lacks valid authentication credentials for the requested resource. This status code is sent with an HTTP WWW-Authenticate response header that contains information on the authentication scheme the server expects the client to include to make the request successfully.

The HTTP 403 Forbidden client error response status code indicates that the server understood the request but refused to process it. This status is similar to 401, except that for 403 Forbidden responses, authenticating or re-authenticating makes no difference. The request failure is tied to application logic, such as insufficient permissions to a resource or action.

The HTTP 423 Locked client error response status code indicates that a resource is locked, meaning it can't be accessed. Its response body should contain information in WebDAV's XML format.

[5e-Cleric]

That seems like a good idea.

[ericscheid]

Though I am slightly worried by this (a MUST requirement)

This status code is sent with an HTTP WWW-Authenticate response header that contains information on the authentication scheme the server expects the client to include to make the request successfully.

No, wait .. I was thinking this is the header that cause the browser to prompt for BASIC authentication.

It can also be things like Bearer for JWT cookies:

WWW-Authenticate: Bearer realm="naturalcrit", error="invalid_token", error_description="The access token expired"

OK, 401 FORBIDDEN, with something like that.

[5e-Cleric]

Though I am slightly worried by this (a MUST requirement)

This status code is sent with an HTTP WWW-Authenticate response header that contains information on the authentication scheme the server expects the client to include to make the request successfully.

No, wait .. I was thinking this is the header that cause the browser to prompt for BASIC authentication.

It can also be things like Bearer for JWT cookies:

WWW-Authenticate: Bearer realm="naturalcrit", error="invalid_token", error_description="The access token expired"

OK, 401 FORBIDDEN, with something like that.

I am not sure if the final conclusion of your post is that i should include that or not

[ericscheid]

I am not sure if the final conclusion of your post is that i should include that or not

yes, do eet.

• return 401 FORBIDDEN
• include WWW-AUTHENTICATE header

[5e-Cleric]

I am not sure if the final conclusion of your post is that i should include that or not

yes, do eet.

[ ] return 401 FORBIDDEN [ ] include WWW-AUTHENTICATE header

do we really want people having to enter username and password there? because that triggers the basic auth

[ericscheid]

do we really want people having to enter username and password there? because that triggers the basic auth

Only if the WWW-AUTHENTICATE header has Basic as the type. We're using Bearer, that won't trigger the browser user/pass dialog (to my knowledge).

[Gazook89]

I just want to clarify the intent with the PR: this doesn't just block "Clone to New", but also prevents viewing the source and download routes? Basically, lock it down for people who don't want the source visible at all (beyond using the browser dev tools).

Philosophically, does a majority of users/contributors want the ability to hide their source code?

Personally, I prefer that all published brews can be studied and/or cloned. If a user doesn't want it cloned, they can choose to not Publish on homebrewery, generate a PDF, and then share that PDF. If we have an option to 'lock it down', we are still powerless to hide the browser Inspect tool, which to me is sort of an 'unfulfilled promise'.

However, take that with a boat-sized cube of salt, since I'm not really a brew creator and so it's pretty low-stakes for me. I understand that certainly some more professional users would like to prevent copying to whatever degree possible.

[Gazook89]

Also, the screenshots for the examples above are a little confusing. You say

The new share page if the brew's cloning is allowed or if you are the author:

But the image is of the /source/ page. And the brew title of the example is "test brew with cloning not allowed", which is contrary to what you are trying to display, I think.

---

This PR also displays Download and Source the same, though still separate routes. It's presented as a box inside a box. I'm not sure this is an improvement over the plain text of the current /source/ page, or the single-click Download button. One point in favor of this new display is that a user has a better idea of what they are downloading-- I imagine some users think the current Download button is actually for downloading the brew, rather than just text.

You mention it is done this way in order to handle authentication?

What about keeping the /source/ page as-is, the plain text with nothing else, just a route accessible only via the address bar. But then doing something similar to this PR as an improvement to the /share/ page....

Meaning, clicking the "Source" button wouldn't navigate you away from /share/, but the brew would be replaced by basically your page with the source box on it. It would have a 'dismiss' or 'x' to close it, and show the brew pages again.

Just a thought. I know that is a big change from what you have currently.

[ericscheid]

And the brew title of the example is "test brew with cloning not allowed", which is contrary to what you are trying to display, I think.

The title should probably be "test brew with cloning not allowed by non-authors" for expository reasons.

I'm not sure this is an improvement over the plain text of the current /source/ page

Technically, the /source/ page is an html page with styles that makes it look like plain text. This was likely done because some old browsers would sniff the content, see html tags, and treat it as if it was html .. despite the Content-Type header saying it is actually text/plain.

The /source/ page should ultimately present text in a window which facilitates a simple Select-All, Copy workflow. (Yes, I do see the Copy and Download buttons. Note too that this presentation layer also breaks the old "backup brew javascript" that was written long ago.)

This PR also displays Download and Source the same, though still separate routes

The /download/ URI should/must provide the brew content as raw text with a Content-Disposition header (to indicate to any browser receiving that to save the received content as a download file). It should only not do that and instead present a html page if for some reason the server cannot or will not provide it as a download (e.g. 401 FORBIDDEN, or 404 NOT FOUND, etc).

It is possible to have a "download" button that constructs in-browser a blob which is then targetted by a hidden link and trigger a download via a simulated click link. Or a simple button that has an actual URI that links to the server that responds with a resource with a Content-Disposition header — in which case we'd need a route that actually does that which is different from this /download/ route we've hijacked to present a html UI.

Given that, and given that the simpler solution of /download/ URI results in an actual download also supports 3rd party javascripts (e.g. one-click "save this brew" or "save all brews" javascripts) .. I would say go with the more open and transparent "/download/ means download" architecture.

[ericscheid]

this doesn't just block "Clone to New", but also prevents viewing the source and download routes

That is my understanding.

It should also block the /css/ route too.

[5e-Cleric]

I just want to clarify the intent with the PR: this doesn't just block "Clone to New", but also prevents viewing the source and download routes? Basically, lock it down for people who don't want the source visible at all (beyond using the browser dev tools).

Yes, it started with me preventing users to clone by simply removing the clone button, and it kinda grew from there when i realized those two other pages also would allow cloning.

Philosophically, does a majority of users/contributors want the ability to hide their source code?

Every user that only shares the pdf and not the share page would prefer their source code protected i would guess.

Personally, I prefer that all published brews can be studied and/or cloned. If a user doesn't want it cloned, they can choose to not Publish on homebrewery, generate a PDF, and then share that PDF. If we have an option to 'lock it down', we are still powerless to hide the browser Inspect tool, which to me is sort of an 'unfulfilled promise'.

We could link the cloning and source availability to published status, but i am not sure they should be linked, a user might want their brew available to viewing in the Vault, but not want it cloned (platform vs tool talk here)

[5e-Cleric]

Also, the screenshots for the examples above are a little confusing. You say

The new share page if the brew's cloning is allowed or if you are the author:

But the image is of the /source/ page. And the brew title of the example is "test brew with cloning not allowed", which is contrary to what you are trying to display, I think.

Yes, sorry, we would probably want to create a name for this, see, this PR disables the nav source dropdown, i can add a screencap of the nav without the button, but doesn't really give you any info, just the nav without a menu.

This PR also displays Download and Source the same, though still separate routes. It's presented as a box inside a box. I'm not sure this is an improvement over the plain text of the current /source/ page, or the single-click Download button. One point in favor of this new display is that a user has a better idea of what they are downloading-- I imagine some users think the current Download button is actually for downloading the brew, rather than just text.

I can delete one of the buttons, and indicate you can download from source. But i am not sure we want to delete the two routes just yet, since i seem to recall some other platform was using our download or source route to get brew info. Should probably wait for #3481 to be merged before deleting routes.

About the looks, i think the current source is almost unnaceptable, at least the navigation bar should be there, for users to navigate back, it is still a part of our tool. I picked the UI page because it was the closest we had to an empty page where to put the code.

You mention it is done this way in order to handle authentication?

It started that way, but the feeling that this should use our UI has turned out to be the strongest point, i figured out how to do the auth filter from App.js

What about keeping the /source/ page as-is, the plain text with nothing else, just a route accessible only via the address bar. But then doing something similar to this PR as an improvement to the /share/ page....

Not a terrible idea, but do see my point above

Meaning, clicking the "Source" button wouldn't navigate you away from /share/, but the brew would be replaced by basically your page with the source box on it. It would have a 'dismiss' or 'x' to close it, and show the brew pages again.

Just a thought. I know that is a big change from what you have currently.

Not really, it'll take me some time though.

[5e-Cleric]

this doesn't just block "Clone to New", but also prevents viewing the source and download routes

That is my understanding.

It should also block the /css/ route too.

good point, alright, given all the talk, i will move the download route back, but keep the download button in the source page.

[5e-Cleric]

this doesn't just block "Clone to New", but also prevents viewing the source and download routes

That is my understanding.

It should also block the /css/ route too.

There is a problem with that. If a user has set a brew to share themes via the theme tag, which is the only way(i think) of using those routes, why would we block the css resource?