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

Add note for MacOS 11+ users blocked by SIP #401

Merged
merged 3 commits into from
Jul 14, 2023

Conversation

keywordnew
Copy link
Contributor

@keywordnew keywordnew commented Apr 28, 2023

A workaround for an issue related to SIP imposed on Docker has been identified in #371. This PR updates README.md to include friendly instructions for MacOS 11+ users blocked by this issue.

PR created at the request of @apyrgio in #371 (comment).

To include these instructions in the current README.md, I judged it most appropriate to nest it under a new FAQ section. Please feel free to refactor as needed.

Closes #371.

@keywordnew keywordnew marked this pull request as ready for review April 28, 2023 13:20
Copy link
Contributor

@apyrgio apyrgio left a comment

Choose a reason for hiding this comment

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

Hi @keywordnew. Thanks a lot for the PR, and sorry for the review delay. I needed to sort some things out post 0.4.1 release.

While looking at our instructions, I realized something:

  • In Dangerzone 0.4.1, before Docker becomes involved in the conversion process, Dangerzone copies the untrusted file (e.g., from Downloads) to a temporary directory. Then, Docker bind mounts it from the temporary directory into the container.
  • In Dangerzone 0.4.0, Docker mounts it directly from the original location (e.g., from Downloads).

In my system, this chain of events does not trigger SIP or produce an error (I fail to see why), but things may be different in your system. Also, this change on the location of the document affects the instructions of this PR. I would assume that Dangerzone, not Docker, would now be the program that would need to access the file under Downloads.

So, would you mind checking once more these instructions for Dangerzone 0.4.1, to make sure they are up-to-date?

README.md Show resolved Hide resolved
@apyrgio
Copy link
Contributor

apyrgio commented May 17, 2023

Hi @keywordnew, gentle ping for the review comments above. Unfortunately I can't reproduce these instructions (and the original issue) in my MacOS device, that's why I'd like your feedback on this.

@keywordnew
Copy link
Contributor Author

Thanks for letting me know to reprioritize this, @apyrgio.

"checking once more these instructions for Dangerzone 0.4.1, to make sure they are up-to-date"

Below are findings from two conversion flows: CLI and GUI for Dangerzone 0.4.1

CLI

pre: disable "Privacy & Security" -> "Files and Folders" -> parent directory of test.pdf

  1. initiate conversion of test.pdf
  2. conversion successful
GUI

pre: disable "Privacy & Security" -> "Files and Folders" -> parent directory of test.pdf

  1. initiate conversion of test.pdf
  2. modal triggered: "“Dangerzone.app” would like to access files in your Downloads folder."
  3. deny request
  4. file picker appears, test.pdf can be selected
  5. conversion apparently successful

@keywordnew
Copy link
Contributor Author

keywordnew commented May 18, 2023

In view of #401 (review) and #401 (comment), this PR with instructions to handle permissions-related UX issues needs amending or withdrawal.

  1. The instructions in the PR would now be useful only to users using Dangerzone 0.4.0 and lower.
  • Does the project have metrics on the percentage of users on older versions of Dangerzone? Keeping the instructions would be a way to avoid difficult user experiences, if a non-trivial cohort of users do not regularly update their installed version. Otherwise, I can withdraw this PR.
  1. As permissions do not actually seem necessary when using Dangerzone 0.4.1, instructions to deny the permissions modal triggered by a GUI-based conversion could be added to the PR.
  • Or made to replace the existing content of the PR, depending on the decision on item 1 above.

@apyrgio
Copy link
Contributor

apyrgio commented May 18, 2023

I see, so you're saying that the modal for SIP does not actually prevent Dangerzone from accessing the file. It's like the user's choice is inconsequential. That's... weird.

In any case, this is something that we can monitor in this issue tracker, and gather user feedback.

The instructions in the PR would now be useful only to users using Dangerzone 0.4.0 and lower.

This seems to be the case. Still, I would prefer if affected users (ideally, every user) upgraded their Dangerzone installation to the latest one. It contains security fixes and improvements that are important.

If Dangerzone 0.4.0 users reach our GitHub page, I believe the most important thing is to see that there's a new release, rather than circumvent an issue locally.

Does the project have metrics on the percentage of users on older versions of Dangerzone?

Nope, we don't have any way to track installed versions, nor a way to inform users about updates, aside from the GitHub releases and our Mastodon channel. This is something that we should solve at some point in a privacy-respecting way.

As permissions do not actually seem necessary when using Dangerzone 0.4.1, instructions to deny the permissions modal triggered by the GUI-based conversion of Dangerzone 0.4.1 could be added to the PR.

The thing is, I can't wrap my head around this choice, and I think that users will be confused as well. We don't have a good explanation of what's the consequence of their action, in order to help them decide. If we did, I'd be more than fine with adding such an instruction :-/.

@keywordnew keywordnew marked this pull request as draft June 30, 2023 16:28
@keywordnew
Copy link
Contributor Author

If Dangerzone 0.4.0 users reach our GitHub page, I believe the most important thing is to see that there's a new release, rather than circumvent an issue locally.

May or may not be the best path, but seemed reasonable to add directions to update the app as the first entry in the FAQ.

Changed PR to draft since a course of action is still unclear.

@apyrgio
Copy link
Contributor

apyrgio commented Jul 4, 2023

Hey @keywordnew. I like the fact that you put first and foremost that users should upgrade to the latest version, and then offer a potential solution. I think this is a good way forward.

I'll be mostly out this week, so I'll manage to see your PR from next week. But I believe we have something that we can merge :-).

@keywordnew
Copy link
Contributor Author

Glad the PR can be made useful, @apyrgio. Just to note, the commits on the PR branch are atomic only for my benefit. So, if a decision is made to merge, a squash would clean up the history :-)

A workaround to an issue related to SIP imposed on Docker has been identified in freedomofpress#371. Update README.md to include friendly instructions for MacOS 11+ users blocked by this issue.
Maintainers have indicated preference for encouraging users to update app version rather than use workarounds to fix issues.
@apyrgio apyrgio force-pushed the 410-macos-sip-instructions branch from 098cee4 to a1bbcdf Compare July 14, 2023 12:23
@apyrgio apyrgio marked this pull request as ready for review July 14, 2023 12:55
@apyrgio apyrgio merged commit a1bbcdf into freedomofpress:main Jul 14, 2023
@apyrgio
Copy link
Contributor

apyrgio commented Jul 14, 2023

Thanks a lot @keywordnew for improving the PR and offering these MacOS instructions. Even if the issue you encountered in 0.4.0 does no longer bite our users in 0.4.1+, it's nice to have SIP instructions somewhere in our project. Also, the fact that we stress that users must always upgrade when they encounter an error is a very welcome addition.

Glad the PR can be made useful, @apyrgio. Just to note, the commits on the PR branch are atomic only for my benefit. So, if a decision is made to merge, a squash would clean up the history :-)

Sigh, I was thinking of squashing the commits, but I didn't want to be too intrusive. Unfortunately I also forgot about this message. Anw, that's ok, the extra commit is not that bad.

@keywordnew
Copy link
Contributor Author

Sigh, I was thinking of squashing the commits, but I didn't want to be too intrusive. Unfortunately I also forgot about [message suggesting squashing]. Anw, that's ok, the extra commit is not that bad.

I appreciate the thoughtful repo gardening. FWIW, I would have done the same.

@keywordnew keywordnew deleted the 410-macos-sip-instructions branch July 19, 2023 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

consistently on convert, invalid json returned from container
2 participants