✨ Adds System.login #4800

miketheman · 2017-08-06T14:41:15Z

Uses current conventions.

Split out from #2368

Signed-off-by: Mike Fiedler miketheman@gmail.com

RX14

Also, can we not use emoji in commit messages, it adds absolutely no information.

RX14 · 2017-08-06T14:55:13Z

spec/std/system_spec.cr

+
+  describe "login" do
+    it "returns String or nil" do
+      System.login.should be_a(String | Nil)


Please test this by comparing the result with echo $USER or similar.

RX14 · 2017-08-06T14:57:08Z

src/crystal/system.cr

@@ -11,6 +11,7 @@ module Crystal
 end


Please add a commented out spec for the Crystal::System.login method.

RX14 · 2017-08-06T14:57:39Z

src/system.cr

@@ -22,4 +22,14 @@ module System
  def self.cpu_count
    Crystal::System.cpu_count
  end
+
+  # Returns the short user name of the currently logged in user or `nil` if it
+  # can't be determined. Note that this information is not secure.


Please clarify on what "secure" means here.

Additionally, when can this return nil? I don't see a reason to declare it as nillable.

Re: nil, there's no guarantee that getlogin returns a string, it can return NULL. That's the source of the comment - so I'm not certain how to handle that particualr behavior.

This is what happens when getlogin returns a NULLcurrently: https://carc.in/#/r/2hbo. You need to manually convert it to a nil, String.new won't do that for you.

Uses current conventions. Split out from crystal-lang#2368 Signed-off-by: Mike Fiedler <miketheman@gmail.com>

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

RX14 · 2017-08-06T15:33:25Z

spec/std/system_spec.cr

+    it "returns String or nil" do
+      shell_login = `echo $USER`.strip
+      login = System.login
+      login.should be_a(String)


We typically don't check return types in spec. If you want to be sure of the returned type, annotate the method.

RX14 · 2017-08-06T15:34:43Z

src/crystal/system.cr

@@ -7,10 +7,15 @@ module Crystal
    # Returns the number of logical processors available to the system.
    #
    # def self.cpu_count
+
+    # Returns the short user name of the currently logged in user.
+    #


I think that the comment should be adjacent to the def self.login as with hostname. Please can you also modify cpu_count at the same time, as this is a tiny change not worth a new PR.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

miketheman · 2017-08-06T15:49:00Z

spec/std/system_spec.cr

+
+  describe "login" do
+    it "returns String when found" do
+      shell_login = `echo $USER`.strip


@RX14 Now that I've got the logic to return Nil, I'm not certain how to structure this test case to handle the current CI - travis has no $USER

According to travis doc there is a $USER env var on all builds: https://docs.travis-ci.com/user/environment-variables/#Default-Environment-Variables
It just states that the value may not always be travis.

$USER should be universal to all unix systems.

All true, yet the returned value of $USER inside the docker runtime on Travis returns null.

No, it seems correct. $USER is "" or unset, and System.login is Nil. It seems the bug is in the spec for not equating "" and Nil. It also just occured to me that `echo $USER`.strip is a weird way to get the value when we have ENV["USER"]. Using ENV["USER"]? would give the correct behaviour of being Nil when unset.

The login name is Nil because the specs are running inside a docker container - which has no login session. That brings us to another question, why is this method useful? It doesn't tell us the current effective user of the process, just the login session.

@RX14 I was using your recommendation from #4800 (comment) .
I'll change to using ENV.

As to the utility, if I log in using user1 and later do sudo su - user2, System.login will return user1, whereas ENV["USER"] will return user2.

For process-specific owner lookup, an enhancement to Process implementing the getuid call would do what you're asking about.

BTW you need ENV["USER"]? to handle the case the env var isn't set.

Oh, just realised. `logname`.strip is the command equivalent to getlogin, not ENV["USER"]?. Sorry for making you jump through hoops! You'll also need to convert "logname: no login name" to nil to pass the test on travis.

straight-shoota · 2017-08-06T16:03:42Z

src/crystal/system/unix/login.cr

+module Crystal::System
+  def self.login
+    if LibC.getlogin
+      String.new(LibC.getlogin)


Wouldn't it be better to store the result in a local variable instead of calling LibC.getlogin twice?

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

RX14 · 2017-08-07T12:35:11Z

I think we should merge a PR to bind cuserid or similar syscall too, and explain the differences. But I don't want to hold up this PR.

miketheman · 2017-08-07T12:37:12Z

@RX14 Quote from man page:

Nobody knows precisely what cuserid() does; avoid it in portable programs. Or avoid it altogether: use getpwuid(geteuid()) instead, if that is what you meant. Do not use cuserid().

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

straight-shoota · 2017-08-07T12:50:17Z

spec/std/system_spec.cr

+  describe "login" do
+    it "returns current session login" do
+      shell_login = `logname`.strip
+      shell_login = nil if shell_login.match(/no login name/)


Please check the exit status, not an arbitrary error message. POSIX specifications demand to provide a diagnostic message but it might not always be (or contain) no login name. But logname must return non-zero exit status if no login is available - thats easier and faster anyway.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

miketheman · 2017-08-07T12:58:04Z

Happy to rebase once we're green and people are 👍 with the changeset.

ysbaddaden · 2017-08-08T08:39:41Z

src/crystal/system/unix/login.cr

+    if login = LibC.getlogin
+      String.new(login)
+    else
+      nil


You don't need the else nil branch.

No, you don't. But I think it looks nicer and is clearer as to what's going on.

If a condition is true then return something, otherwise nothing, aka nil in crystal terms. Adding else nil is just noise.

It seems else nil is used only once in the entire repo: https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/semantic/call_error.cr#L288 And this should probably be removed as well.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

miketheman · 2017-08-10T10:53:23Z

Anything else for me to do here?

miketheman · 2017-08-15T11:37:41Z

@RX14 Anything remaining here, or can this be merged?

ysbaddaden · 2017-08-15T12:25:50Z

I believe there is nothing more to do. I'm just dubious if we really need this, personally.

RX14 · 2017-08-15T15:05:35Z

I think this and a PR to access the processes' effective uid/gid should be merged at the same time so that their respective documentation can reference each other and their difference. I think that accessing the effective uid/gid will be much more common and useful than this method.

miketheman · 2017-09-28T12:53:38Z

@ysbaddaden If this isn't desirable, that's fine - let's make a decision and get on with it. 😀

This being split out from #2368 - It appeared to be desired, that's why I took it on. Feedback on the desirability of this has been spotty at best.

@RX14 It is entirely possible that you're right, and that other extensions may be more useful - I didn't try to conquer those, so this remains waiting for someone to decide what its future should be.

I'm happy to consider other directions, just need some feedback on what to do about this PR.

straight-shoota · 2018-04-23T09:05:49Z

Could this get revisited now that #5627 is getting ready to be merged?

straight-shoota · 2021-12-29T15:08:25Z

Closing stale PR. There has been no activity for a long time.

This is probably still worth implementing, but needs a fresh start to discuss the API and integrate with existing APIs (see #7738).

RX14 requested changes Aug 6, 2017

View reviewed changes

miketheman added 2 commits August 6, 2017 11:20

Adds System.login

5b61f89

Uses current conventions. Split out from crystal-lang#2368 Signed-off-by: Mike Fiedler <miketheman@gmail.com>

Update with feedback

c109496

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

miketheman force-pushed the miketheman/system-login branch from 1af3125 to c109496 Compare August 6, 2017 15:21

RX14 requested changes Aug 6, 2017

View reviewed changes

More feedback

f75e8f3

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

miketheman commented Aug 6, 2017

View reviewed changes

straight-shoota reviewed Aug 6, 2017

View reviewed changes

miketheman added 4 commits August 7, 2017 07:54

Reduce calling LibC.getlogin by one

83ba8c7

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

Use envvar in spec test

333f9c0

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

Return Nil if envvar not set

8588b35

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

Return a nil value, not a Nil type

4123da4

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

Use logname to detect

15514e9

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

straight-shoota reviewed Aug 7, 2017

View reviewed changes

Use status code instead of string match

ac63910

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

RX14 approved these changes Aug 7, 2017

View reviewed changes

ysbaddaden reviewed Aug 8, 2017

View reviewed changes

Remove nil condition, per feedback

23ea8d5

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

RX14 approved these changes Aug 8, 2017

View reviewed changes

Merge branch 'master' into miketheman/system-login

bb25642

straight-shoota mentioned this pull request Apr 5, 2018

Add Etc module with methods for fetching user information #2368

Closed

straight-shoota closed this Dec 29, 2021

straight-shoota added kind:feature topic:stdlib:system labels Dec 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Adds System.login #4800

✨ Adds System.login #4800

miketheman commented Aug 6, 2017

RX14 left a comment

RX14 Aug 6, 2017

RX14 Aug 6, 2017

RX14 Aug 6, 2017

RX14 Aug 6, 2017

miketheman Aug 6, 2017

RX14 Aug 6, 2017 •

edited

RX14 Aug 6, 2017

RX14 Aug 6, 2017

miketheman Aug 6, 2017

straight-shoota Aug 6, 2017

RX14 Aug 6, 2017

miketheman Aug 7, 2017

RX14 Aug 7, 2017

miketheman Aug 7, 2017

RX14 Aug 7, 2017

miketheman Aug 7, 2017

RX14 Aug 7, 2017 •

edited

straight-shoota Aug 6, 2017

RX14 commented Aug 7, 2017 •

edited

miketheman commented Aug 7, 2017

straight-shoota Aug 7, 2017

miketheman commented Aug 7, 2017

ysbaddaden Aug 8, 2017

RX14 Aug 8, 2017

ysbaddaden Aug 8, 2017

straight-shoota Aug 8, 2017

miketheman commented Aug 10, 2017

miketheman commented Aug 15, 2017

ysbaddaden commented Aug 15, 2017

RX14 commented Aug 15, 2017

miketheman commented Sep 28, 2017

straight-shoota commented Apr 23, 2018

straight-shoota commented Dec 29, 2021

✨ Adds System.login #4800

✨ Adds System.login #4800

Conversation

miketheman commented Aug 6, 2017

RX14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 Aug 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 Aug 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 commented Aug 7, 2017 • edited

miketheman commented Aug 7, 2017

Choose a reason for hiding this comment

miketheman commented Aug 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miketheman commented Aug 10, 2017

miketheman commented Aug 15, 2017

ysbaddaden commented Aug 15, 2017

RX14 commented Aug 15, 2017

miketheman commented Sep 28, 2017

straight-shoota commented Apr 23, 2018

straight-shoota commented Dec 29, 2021

RX14 Aug 6, 2017 •

edited

RX14 Aug 7, 2017 •

edited

RX14 commented Aug 7, 2017 •

edited