Crystal: System::User and System::Group API

[straight-shoota]

There have been some PR's adding System::User and System::Group features, originally #5615 then it's reboot #5627 and now there's #7725 with a subset of the features, which is much easier to review.

We've had discussions about the design of these APIs directly in the PRs, but that's not a good procedure, especially when we're talking about quite substantial, completely new APIs.

Instantiation

There have been several proposal for getting instances of System::{Group,User}. The last state from #5627 was .get(name_or_id), .from_gid(id), .from_groupname(name) and equivalent for User (plus nilable ? methods).

(There were also some methods to retrieve the name from id, or id from name directly (.name, .gid). I'm not sure we really need them and they can easily be added later, so we don't need to discuss them right away.)

7725 now comes with .from_name and .from_id methods for both types (plus nilable ? methods). This is IMO an improvement, because we don't need gid/uid and groupname´/username` explicitly in the method name, when the fact that it's about a group/user is already determined by the type this method is called on.

A generic method for looking up either by name or id might be useful, but this is a feature that can be added as an enhancement later.

Properties

The current proposals are mainly based on exposing properties provided from getgrnam, getpwnam etc. But we need to take a wider look at exposing a portable API that is not POSIX-specific.

5627 exposed the following properties:

User:

• name : String
• uid : String
• raw_uid : UInt32 | String
• gid : String
• raw_gid : UInt32 | String
• home : String
• shell : String
• info : String
• group : Group
• root? : Bool

Group:

• name : String
• gid : String
• raw_gid : UInt32 | String
• user_names : Array(String)
• users : Array(User)

In #7725 we have

User:

• name : String
• password : String
• user_id : LibC::UidT
• group_id : LibC::GidT
• directory : String
• shell : String

Group:

• name : String
• password : String
• id : LibC::GidT
• members : Array(String)

There are already a few differences between both of them, and I think the correct solution is somewhere in between.

The interface in Golang is also a nice reference to a very neat API:

User:

• Uid string
• Gid string
• Username string
• Name string
• HomeDir string
• GroupIds() string[]

Group:

• Gid string
• Name string

I propose to use the exact same properties as used in Golang in our API. They are a good collection of essential features working cross-platform.

That's very minimal, but more features are always more complex. And honestly, you mostly won't need much more anyway. Let's start with this limited but solid set and discuss possible additions separately.

[woodruffw]

:+1: on generally mirroring the Golang APIs for users and groups.

One potential pain point: it looks like Go uses the nonstandard getgrouplist call to retrieve a user's associated groups. All of our (nix-y) targets should have some version of it, but we'll need to confirm that they all behave semi-consistently (or special-case the ones that don't). The alternative is the POSIX getgrent family, but those are non-reentrant/MT-unsafe. There's getgrent_r, but I believe it's glibc only.

[straight-shoota]

When it works for Go, why shouldn't it work for Crystal? They already support more platforms than we do.
And the implementation seems to be pretty straightforward and even if it's not a standard call cross-platform compatible (with only a small adjustment for darwin).

[j8r]

Is it planned that File::Info#owner will return System::User and File::Info#group return System::Group?

[straight-shoota]

That's another issue. Please let's focus on the main API first.

[j8r]

In the API Arrays should be Sets for Group user_names, users and members.

[j8r]

I don't know @straight-shoota , it's not useful to provide an API if there is no other use inside the stdlib. Else, it can be a shard.

[straight-shoota]

In the API Arrays should be Sets for Group user_names, users and members.

Following my proposal these properties should not be exposed in the API at all.

it's not useful to provide an API if there is no other use inside the stdlib.

Being able to retrieve basic user information is already useful by itself and an essential feature that should be provided in stdlib. Whether the User API gets integrated with other APIs is irrelevant. Although it is already planned to integrate it with the Process API.

More in-depth features such as getting more detailed information on users and groups (including group members) and perhaps also modifying these are highly platform-specific and should be supplied by a shard.

[woodruffw]

More in-depth features such as getting more detailed information on users and groups (including group members)

Just FWIW, getting group members is something #7725 supports (it's part of the information returned by the standard getgrnam_r call). But I agree that most things more complicated than that belong in a shard :wink:

I also like the idea @j8r brought up about integrating these classes into File::Info, but agree that the current PR shouldn't attempt that.

As for User#group_ids: sounds good to me! Is that someone we want in #7725, or via a follow-up PR?

[straight-shoota]

Yes, getgrnam_r directly provides group members. But there must be some reason why Golang doesn't expose this. Maybe it's just because their Go-native implementation which doesn't use libc can't figure this out. Maybe there are some portability concerns. I'm not sure. But before adding it, we should make sure we can support it. And it could easily be added later.

As for User#group_ids: sounds good to me! Is that someone we want in #7725, or via a follow-up PR?

I'd rather have it in a follow-up. Let's work on smaller, contained pieces. That's easier.

[woodruffw]

Yes, getgrnam_r directly provides group members. But there must be some reason why Golang doesn't expose this. Maybe it's just because their Go-native implementation which doesn't use libc can't figure this out. Maybe there are some portability concerns. I'm not sure. But before adding it, we should make sure we can support it. And it could easily be added later.

Yeah, this is a bit of a mystery to me. From the Go source:

/*

Package user allows user account lookups by name or id.


For most Unix systems, this package has two internal implementations of

resolving user and group ids to names. One is written in pure Go and

parses /etc/passwd and /etc/group. The other is cgo-based and relies on

the standard C library (libc) routines such as getpwuid_r and getgrnam_r.


When cgo is available, cgo-based (libc-backed) code is used by default.

This can be overridden by using osusergo build tag, which enforces

the pure Go implementation.

*/

So, either way they have access to group memberships. They don't necessarily have to extract them when parsing /etc/group, but they get them for free when using getgrnam_r -- the call only succeeds if a sufficiently large buffer is passed for string storage. I'll dig into the actual implementation and see if I can figure out the reasoning.

I'd rather have it in a follow-up. Let's work on smaller, contained pieces. That's easier.

:+1:

[woodruffw]

So, here's where Go user getgrnam_r:

https://github.com/golang/go/blob/3403ee524bef9f6d2d69e11410fbf16854447d21/src/os/user/cgo_lookup_unix.go#L136

That in turn calls buildGroup, which only pulls the gid and name fields out:

func buildGroup(grp *C.struct_group) *Group {
    g := &Group{
        Gid:  strconv.Itoa(int(grp.gr_gid)),
        Name: C.GoString(grp.gr_name),
    }
    return g
}

So, pretty similar in implementation to our current one. I can't find any documentation online suggesting that the gr_mem member of group is unreliable or unsuitable for cross-platform code, but it's possible I'm missing something.

[straight-shoota]

I like the new lookup methods using named arguments suggested in https://github.com/crystal-lang/crystal/pull/7725#issuecomment-512157450.

But are we settled on .find_by?

I'd prefer simply .find, which is exactly as expressive. IMO the suffix _by doesn't add any value to the name. We don't need to follow ActiveRecord conventions here, this is a completely different domain (and language).

User.find(name: "foo")
User.find(id: 1)

vs.

User.find_by(name: "foo")
User.find_by(id: 1)

It's not a big difference, but a little bit more unnecessary clutter in my view.

/cc @ysbaddaden @woodruffw @bcardiff

[bcardiff]

I like how it reads. In AR is more needed since User.find would be by id. And the using User.find x would depend on the argument been a hash or int. Buy my main argument is how it reads (find by name is proper).

[ysbaddaden]

Find by id and find by name read as plain English, and are just a little more explicit than find id and find name.

[woodruffw]

I'm also 👍 on find_by vs. find, although for different reasons (I didn't use AR much in Ruby) -- find makes me think of an enumerable, whereas find_by makes me think of a query against a resource.

[straight-shoota]

The core System::User and System::Group classes have been implemented in #7725.
Now, we're still missing a few components and integrations with other APIs:

• Integration with File API: File.chown/owner/group
• Integration with Process API (#7829)