search

crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.32k stars 1.61k forks source link

System::User and System::Group API #7738

Open straight-shoota opened 5 years ago

straight-shoota commented 5 years ago

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:

Group:

In #7725 we have

User:

Group:

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:

Group:

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 commented 5 years ago

:+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 commented 5 years ago

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 commented 5 years ago

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

straight-shoota commented 5 years ago

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

j8r commented 5 years ago

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

j8r commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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:

HertzDevil commented 2 years ago

One thing I discovered while trying to get Crystal running on Termux: The pw_passwd and pw_gecos fields are not part of POSIX, and Bionic, Android's C library implementation, declares these fields but always puts the null pointer in them. This means the following will always fail there:

require "system/user"

module Crystal::System::User
  private def from_struct(pwd)
    user = String.new(pwd.pw_gecos).partition(',')[0] # raises ArgumentError here
    new(String.new(pwd.pw_name), pwd.pw_uid.to_s, pwd.pw_gid.to_s, user, String.new(pwd.pw_dir), String.new(pwd.pw_shell))
  end
end

user_name = `id -un`.chomp
System::User.find_by(name: user_name) # Unhandled exception: Cannot create a string with a null pointer (ArgumentError)

I think @name should default to @username if this field is null.

Another question is, there hasn't been an implementation of the user / group APIs on Windows yet. Would these further restrict these interfaces or do we expose as many fields as possible?

HertzDevil commented 2 years ago

A while ago I tried to implement what we have right now, based on Golang:

There is a lot of code for these classes, and it is hard to tell what works and what doesn't on a Windows Home installation. In fact I don't even know if System should stay in the standard library.

Another useful reference, Cygwin: https://github.com/cygwin/cygwin/blob/master/winsup/cygwin/uinfo.cc