Adds System Users and Groups #5627

chris-huxtable · 2018-01-22T18:44:08Z

Attempts to support Darwin, all Linux's, OpenBSD, and FreeBSD

Also adds features like:

Process.owner = System::User.get("nobody")
Process.group = System::Group.get("nobody")
Process.user
Process.group
System::User.get("root").shell
System::Group.get("wheel").members
File.chown("some/path", "root", "wheel")
File.chown("some/path", System::User.get("root"), System::Group.get("wheel"))
a_pointer.to_slice_null_terminated(a_limit) (useful for converting the Char** to an Array(String))

Previous PR #5615

RX14 · 2018-01-22T18:53:18Z

This PR is a rework of #5615.

Ditto all the review comments I made on the last PR, of course.

chris-huxtable · 2018-01-22T19:11:42Z

Just to clarify in this thread. I can removed System.{user, group,user?,group?}.

drosehn · 2018-01-22T19:13:48Z

Does this reflect the distinction between gid/egid and uid/euid?
("reflect" in the sense that all four can be separately inspected/modified via the crystal API)

(later: nevermind. Based on a quick skim over the changes, it looks like it does)

spec/std/process_spec.cr

spec/std/system/group_spec.cr

spec/std/system/user_spec.cr

src/pointer.cr

src/system/group.cr

chris-huxtable · 2018-01-22T19:20:57Z

@drosehn - Process has a method for real, effective and saved users/groups.

src/system/group.cr

src/system/user.cr

straight-shoota · 2018-01-22T19:39:46Z

Just to clarify in this thread. I can removed System.{user, group,user?,group?}.

I guess that's still open for debate. Let's wait to here some thoughts on which of these should be preferred:

System::User.get and System::Group.get (plus ?-methods)
System.user and System.group (plus ?-methods)

There are certainly arguments for both. User.get/Group.get means single responsibility. On the other hand, there is also Process.user. Should this then also be User.current? The method name get is somewhat arbitrary. The reason to avoid new is solid, but it's not entirely natural to use get. In Go, for example, it's User.Lookup.
So I actually tend to favour System.user/System.group because it follows the same scheme as Process.user/Process.group, as well as existing System class methods: System.hostname means "give me the system's hostname", System.user("root")means "give me the system's user with name 'root'".

chris-huxtable · 2018-01-22T19:44:20Z

Instead of {User, Group}.exists?() I am going to implement User.uid?(), Group.gid?(), User.name?(), and Group.name?() as lighter-weight checkers of existence and fetchers of info.

chris-huxtable · 2018-01-22T20:03:28Z

@straight-shoota - @RX14, put it in as changes to be made in the previous PR. I personally wouldn't use it. All I am interested in is making sure I can use System::{User, Group).get().

src/pointer.cr

src/system/user.cr

chris-huxtable · 2018-01-22T20:21:42Z

@straight-shoota - I like System::User.lookup(), System::User.lookup?(), and the Group equivalents but its uncomfortably long. Though I am willing to change it. All being said I still like square brackets best.

RX14 · 2018-01-22T20:26:39Z

Let's freeze changes to this PR and wait a few days for other people to weigh in on how they think the API should work. No more nitpicks on the implementation until we've decided on the interface please!

chris-huxtable · 2018-01-22T20:30:10Z

Sure, though I have some changes to documentation in the pipe.

src/system/group.cr

RX14 · 2018-01-22T20:32:48Z

@chris-huxtable if you've written stuff, push it, but i'd suggest not writing more and waiting for consensus to emerge.

RX14 · 2018-01-22T20:33:25Z

@Sija please make your reviews one review with multiple comments, instead of multiple reviews.

src/system/group.cr

chris-huxtable · 2018-01-22T20:59:24Z

@drosehn - As of right now it doesn't let you set the real/effective/saved uid's independently. It can be done if you want to use LibC. From a security standpoint, someone not understanding the difference will likely screw things up, so I think it best to not make it easy to do so.

chris-huxtable · 2018-01-22T21:51:57Z

I would also like to float the idea of having Process.root?() to see if you are running as root. Which would be more light-weight then Process.user == System::User.get(root)

def self.root?() : Bool
  return LibC.getuid == 0
end

src/pointer.cr

src/system/group.cr

drosehn · 2018-01-23T01:43:52Z

As of right now it doesn't let you set the real/effective/saved uid's independently. It can be done if you want to use LibC. From a security standpoint, someone not understanding the difference will likely screw things up, so I think it best to not make it easy to do so.

Well, that seems less-than-wonderful to me, but then I'm a systems programmer who has worked on Solaris, MacOS, AIX (ugh), and Linux for many years. I already wrote a crystal program which uses these features, because I know what I'm doing with the different values. I stopped after implementing what I needed, because I wasn't sure how I'd write specs for setting these values, when you have to be root in order to set them. I trust me with root access on my machines, but I'm not sure how could write specs which would need to be run as root!

(note: i have no experience in writing specs for a compiler's standard library)

chris-huxtable · 2018-01-23T01:48:54Z

@drosehn - I don't have specs for any of the setuid/et al. methods because it would be dangerous to run as root. What I may do though is make some that catch the raises which would be thrown when they fail. This would at least test for the failing cases.

NOTE: Breaking Change. Removes `File.chown` with named arguements *uid* and *gid*.

chris-huxtable · 2018-08-26T03:33:12Z

Ready for review, provided this passes tests (which they should).

straight-shoota · 2018-08-26T18:58:59Z

spec/std/process_spec.cr

@@ -253,4 +253,78 @@ describe Process do
      Process.find_executable("some_very_unlikely_file_to_exist").should be_nil
    end
  end
+
+  describe "users and groups" do
+    it "has a user" do


Just naming these examples after the methods they're testing more clearly communicates what they're about: it "#user" do.

Ditto all below.
The (failing) setter specs could also be grouped in a describe "#user=" do (etc.)

straight-shoota · 2018-08-26T19:06:06Z

src/file.cr

  # ```
-  def self.chown(path, uid : Int = -1, gid : Int = -1, follow_symlinks = false)
-    Crystal::System::File.chown(path, uid, gid, follow_symlinks)
+  def self.chown?(path : String, owner : UserRep? = nil, group : GroupRep? = nil, follow_symlinks : Bool = false) : Bool


It is necessary, but not a necessary part of this PR. This is already very complex, has changed a lot of times and is really hard to review. Please, you would do everyone a favour if you hold the chown change back until the rest has been merged. It's just so much easier to deal with smaller pieces one after the other than chew everything at once.

straight-shoota · 2018-08-26T19:19:44Z

We can add an explicit getter for named and id based values. But that seems kludgy.

There is no way around something like this. We can't distinguish ids from names by data type if both are strings. And there is no way to distinguish by value because according to POSIX definition, any id should also be a valid name.

One alternative is to have separate methods from_uid/from_username. The other option is to use the same method name but different argument names: get(gid: "123")/get(name: "root").

j8r · 2018-08-26T22:50:42Z

What about extending the API with Int overloads for Unix systems? IDs are a feature that Windows doesn't have.

For usernames, the API can be the same because this feature is in common.

Sija · 2018-08-26T23:29:34Z

src/crystal/system/unix/group.cr

+  end
+
+  # Indicates if the given gid is valid
+  def self.valid_gid?(gid : Int?) : Bool


Why allowing nillable gid argument? Ditto all (gid/uid) below.

Nil is not a valid uid and allows the check to be done for the user implicitly.

Why any1 would pass a nil as a gid/uid?
Same can be said about other types as well.

Since a lot of the backend is nilible (invalid uid) it helps keep everything clean.

I've counted 2 places (https://github.com/crystal-lang/crystal/pull/5627/files#diff-498ff69d4645ddc11a8ea3ba27be9aa8R139 & https://github.com/crystal-lang/crystal/pull/5627/files#diff-75e18fac160b2c9c267ae8b2e112e2fdR160) where it would needed to be handled, that's not a lot. I'd argue that a clean API here is more important. Same goes for valid_uid?.

Fair. I will think about it. It was used more, but I guess I have reduced them out.

Sija · 2018-08-26T23:32:05Z

src/file.cr

@@ -237,23 +240,34 @@ class File < IO::FileDescriptor
    basename(path).chomp(suffix)
  end

+  private alias UserRep = System::User | String | UInt32 | Int32


I'd rather avoid using private aliases like these, in docs they'll end up pretty cryptic for end-user. Also, the name is not easily understable (I guess Rep suffix stands for Representation but still, not everyone might grok that).

I can replace them with the long form, but the lines become awfully long.

IMO it's better than an unknown, cryptic alias.

We could add

alias System::User::Representation = System::User | String | UInt32 | Int32

Sija · 2018-08-26T23:34:11Z

src/lib_c/arm-linux-gnueabihf/c/unistd.cr

@@ -8,6 +8,8 @@ lib LibC
  X_OK                =  1
  SC_CLK_TCK          =  2
  SC_NPROCESSORS_ONLN = 58
+  # FIXME: SC_GETGR_R_SIZE_MAX = 69???
+  # FIXME: SC_GETPW_R_SIZE_MAX = 70???


Are these still relevant?

Those should be fixed. They are need for that platform and I haven't been able to verify them. Does anyone know or use that platform and can extract the correct values from the includes?

The enum declaration is identical for all glibc archs I checked (i686, x86_64 and armeabi), so they should be 69 and 70 too on arm-linux-gnueabihf too.

With that I will set them as such. I just haven't managed to personally witness it with my own eyes.

straight-shoota · 2018-10-18T08:54:41Z

@chris-huxtable It seems we're waiting for you to address some of the review comments and rebase on master.

chris-huxtable · 2018-10-18T14:48:45Z

@straight-shoota - I haven’t had time. I am hopefully going to have some down time in the coming weeks.

woodruffw · 2019-04-29T01:01:21Z

JFYI: I'm going to try to revive a trimmed-down version of this (just System::Group and System::User).

bcardiff · 2019-07-27T14:34:21Z

Closed by #7725