-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds System Users and Groups #5627
Conversation
This PR is a rework of #5615. Ditto all the review comments I made on the last PR, of course. |
Just to clarify in this thread. I can removed System.{user, group,user?,group?}. |
Does this reflect the distinction between (later: nevermind. Based on a quick skim over the changes, it looks like it does) |
@drosehn - |
I guess that's still open for debate. Let's wait to here some thoughts on which of these should be preferred:
There are certainly arguments for both. |
Instead of |
@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 |
@straight-shoota - I like |
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! |
Sure, though I have some changes to documentation in the pipe. |
@chris-huxtable if you've written stuff, push it, but i'd suggest not writing more and waiting for consensus to emerge. |
@Sija please make your reviews one review with multiple comments, instead of multiple reviews. |
@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 |
I would also like to float the idea of having def self.root?() : Bool
return LibC.getuid == 0
end |
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 (note: i have no experience in writing specs for a compiler's standard library) |
@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*.
083ceae
to
7c72ae3
Compare
Ready for review, provided this passes tests (which they should). |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
# ``` | ||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
What about extending the API with For usernames, the API can be the same because this feature is in common. |
end | ||
|
||
# Indicates if the given gid is valid | ||
def self.valid_gid?(gid : Int?) : Bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why allowing nillable gid
argument? Ditto all (gid
/uid
) below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nil
is not a valid uid and allows the check to be done for the user implicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why any1 would pass a nil
as a gid
/uid
?
Same can be said about other types as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since a lot of the backend is nilible (invalid uid) it helps keep everything clean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair. I will think about it. It was used more, but I guess I have reduced them out.
@@ -237,23 +240,34 @@ class File < IO::FileDescriptor | |||
basename(path).chomp(suffix) | |||
end | |||
|
|||
private alias UserRep = System::User | String | UInt32 | Int32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can replace them with the long form, but the lines become awfully long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's better than an unknown, cryptic alias.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add
alias System::User::Representation = System::User | String | UInt32 | Int32
@@ -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??? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these still relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With that I will set them as such. I just haven't managed to personally witness it with my own eyes.
@chris-huxtable It seems we're waiting for you to address some of the review comments and rebase on master. |
@straight-shoota - I haven’t had time. I am hopefully going to have some down time in the coming weeks. |
JFYI: I'm going to try to revive a trimmed-down version of this (just |
Closed by #7725 |
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