audit: ignore group write bit

[alexreg]

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes? Here's an example.
• Have you successfully run brew style with your changes locally?
• Have you successfully run brew tests with your changes locally? (Well, actually no, but I get failures normally, and PR introduces no new ones as far as I can tell.)

---

Fixes #7068.

[alexreg]

This PR is in response to the above issue and specifically #7068 (comment). Please let me know if this is the sort of solution you'd like. (It seems reasonable to me, and fairly conservative.)

[MikeMcQuaid]

Looks good so far, a few tweaks.

[MikeMcQuaid]

Can this be simplified a bit? It's a bit hairy as-is.

[alexreg]

Yeah, it is... Sadly, I don't think so. The obvious way to understand it is that actual_mode & 0020 select the group write bit from the actual file mode (i.e., gives either 0020 or 0000) and then sets the corresponding bit on 0100644 if so. i.e., it takes 0100644 and adds the group write bit if and only if the group write bit is set on the actual fine -- which makes it 0100664 instead. I think it's right that & ~File.umask should remain as the outermost bit operation, right? Anyway, if you want me to put another comment in for this, or rework it into a form you have in mind, let me know. :-)

[MikeMcQuaid]

What about just checking the individual bits?

[alexreg]

@MikeMcQuaid Yep, that occurred to me too, but wanted_mode is used for the diagnostic message just below, which makes this calculation quite convenient I think.

[MikeMcQuaid]

I think the message should change too. It's no longer particularly accurate if we're going to ignore the group bit.

[alexreg]

Ah, the problem is now: what if the above conditions are all satisfied, but the umask is more restrictive? Should we honour that or not? If so, it means a 4th problem message potentially, which awkward, since it would effectively make the previous ones redundant.

[alexreg]

(Per the above, if we want to respect the umask, I think my previous approach works best. Otherwise, we can go with yours.)

[MikeMcQuaid]

I'm not sure I fully understand how we currently honour or do not the umask. Can you explain a case in which it would affect an audit failure and what your opinion is on what we should do? Thanks ❤️

[alexreg]

As I understand, the current behaviour is basically that the umask is used to "mask out" (clear) certain bits when incorrect permissions are found and a chmod is suggested (with a numeric rather than a symbolic argument). The Wikipedia page on umask has an interesting sentence:

Generally, the mask only affects file permissions during the creation of new files and has no effect when file permissions are changed in existing files, however, the chmod command will check the mask when the chmod options are specified using symbolic mode and a reference to a class of users is not specified.

Obviously, we are not dealing with the creation of files here. The question is, is our situation analagous to a symbolic change without a reference to a class of users (i.e., something like +r as opposed to g-w, the latter making a reference to the class of users g). I would say no, so that we can probably just not worry about the umask. What do you think?

[MikeMcQuaid]

I would say no, so that we can probably just not worry about the umask. What do you think?

Agreed!

[alexreg]

Okay, amended per your comments; just left your last comment unresolved for now, so you can let me know your thoughts. Is the above test failure spurious or something? It doesn't seem like it has anything to do with my changes at least.

[alexreg]

@MikeMcQuaid Pushed a new version just now, following our discussion. Hope this is closer to the ideal solution. :-)

[MikeMcQuaid]

brew style is failing but otherwise: yeh, this looks much better and easier to follow. Great work!

[alexreg]

@MikeMcQuaid Thanks. Fixed style issues and pushed.

[MikeMcQuaid]

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @alexreg!

[alexreg]

@MikeMcQuaid No problem; thanks for reviewing.