zfs allow/unallow should work with numeric uid/gid

[avg-I]

And that should work even (especially) if there is no matching user or
group name. The change is originally by Xin Li delphij@FreeBSD.org.

The originally submitted pull request is openzfs/openzfs#690 for the old OpenZFS.
It passed automated tests and it was reviewed and accepted, but for some reason never committed to illumos.

This change should fix issue #9792.
Original illumos bug report: https://www.illumos.org/issues/6037

With this patch I am able to see and remove permission delegations for users that have been deleted after those permissions were delegated.

• Bug fix (non-breaking change which fixes an issue)

• New feature (non-breaking change which adds functionality)

• Performance enhancement (non-breaking change which improves efficiency)

• Code cleanup (non-breaking change which makes code smaller or more readable)

• Breaking change (fix or feature that would cause existing functionality to change)

• Documentation (a change to man pages or other documentation)

• My code follows the ZFS on Linux code style requirements.

• I have updated the documentation accordingly.

• I have read the contributing document.

• I have added tests to cover my changes.

• I have run the ZFS Test Suite with this change applied.

• All commit messages are properly formatted and contain Signed-off-by.

[avg-I]

CC @delphij, @amotin

[ahrens]

Sorry that openzfs/openzfs#690 fell through the cracks. Thanks for picking it up again. FYI, It looks like @prakashsurya sent the RTI, and there was a question from @rmustacc:

[have we] verified that the unknown uid/gid message was printed out?

I see the example output user (unknown: 1002) destroy, which I think is what Robert is asking about but I'm not sure.

[codecov-io]

Codecov Report

Merging #10280 into master will decrease coverage by 0.00%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master   #10280      +/-   ##
==========================================
- Coverage   79.52%   79.51%   -0.01%     
==========================================
  Files         389      389              
  Lines      123120   123123       +3     
==========================================
- Hits        97906    97897       -9     
- Misses      25214    25226      +12     

Flag	Coverage Δ
#kernel	79.92% <ø> (+0.01%)	⬆️
#user	66.04% <50.00%> (+0.02%)	⬆️

Impacted Files	Coverage Δ
cmd/zfs/zfs_main.c	82.50% <50.00%> (+0.01%)	⬆️
cmd/zdb/zdb_il.c	30.86% <0.00%> (-24.08%)	⬇️
module/zfs/space_map.c	93.31% <0.00%> (-4.96%)	⬇️
module/zfs/spa_checkpoint.c	93.78% <0.00%> (-4.35%)	⬇️
module/lua/lmem.c	83.33% <0.00%> (-4.17%)	⬇️
module/icp/api/kcf_mac.c	37.14% <0.00%> (-2.86%)	⬇️
module/zfs/dsl_synctask.c	92.40% <0.00%> (-2.54%)	⬇️
module/zfs/vdev_raidz_math.c	76.57% <0.00%> (-2.26%)	⬇️
module/zfs/lzjb.c	98.14% <0.00%> (-1.86%)	⬇️
module/zfs/zrlock.c	89.23% <0.00%> (-1.54%)	⬇️
... and 57 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ed4391...5d136a9. Read the comment docs.

[avg-I]

I am also not sure if Robert meant that.
Here is how an unknown user ID is reported and used as a -u argument:

$ zfs allow testz/deleg
---- Permissions on testz/deleg --------------------------------------
Local+Descendent permissions:
        user (unknown: 2002) destroy,mount

$ zfs unallow -ld -u 2002 destroy testz/deleg

$ zfs allow testz/deleg
---- Permissions on testz/deleg --------------------------------------
Local+Descendent permissions:
        user (unknown: 2002) mount

[rmustacc]

@ahrens, @avg-I To clarify, the question I was asking was related to error paths with as highlighted above (at least, I think). At the time it seemed like the automated testing didn't cover them, so I was trying to make sure they had been manually verified on illumos by the folks submitting the change. It's not really relevant here, but since @prakashsurya didn't know the answer and Yuri never got back to us, that's why it lingered, unfortunately.

[rmustacc]

On 6/2/20 2:43 PM, Pavel Snajdr wrote: Is '?' a valid username on any of the potential platforms?

Depends on what you mean by valid. While tools like useradd/adduser probably will reject it, you can easily add a user to /etc/passwd or /etc/shadow with a username of '?' and use tools like su and id to tell you about the user. I did that on an Ubuntu install just now as a poc. In general, I would assume that folks can make usernames into just about an arbitrary thing they want based on editing /etc/passwd with varying degrees of that working throughout a broader system.

[avg-I]

Looks like this simple change is getting stuck again.
Can we get it in and then think about usability improvements (and whether they are needed at all) ?

[behlendorf]

@avg-I that sounds like a good plan to me. If the format ends up being a problem we can always address it.

[snajpa]

@behlendorf please forgive me for being off-topic on this one, I don't know of a better channel where to relay this - when I was commenting on some recent PRs, it is really so that even a minor comment can postpone the merging of even the simplest change.

This PR has come to mind today again with a tweet from someone I respect from the OSS world - I think here at OpenZFS, we can relate a lot to that:

https://twitter.com/domenkozar/status/1273537675639894016

I'm sure this is not the best place to lead such a discussion, but I just wanted to leave it somewhere as a note - if there's a better place to open up a discussion about this... I think the best we can do about the nitpicking is to somehow mention that somewhere in the contributors guidelines as a pledge for reviewers to stay most on-point as possible.