include userInGroup check in Lua modules when installation is group-restricted

[damianam]

This enables the use of userInGroup function in lua modules (see #2127).

The test is missing because it is not straight forward to implement. We need to create a module with an associated group, but when loading it it needs to fail (so the user can't belong to the group). If the user can't belong to the group, he/she can't install it using that group. Moreover, the group has to exist. This makes it very difficult to implement a reliable test.

Any idea to implement a meaningful test? Should this feature be optional (which would require adding yet another option to enable/disable it)?

[damianam]

As commented in our last telco (https://github.com/easybuilders/easybuild/wiki/Conference-call-notes-20170719), we could extend the group easyconfig parameter, and make it (optionally) a tuple, with a load error message, allowing customization that way.

Thoughts?

[damianam]

@boegel currently we can't check properly that the module is correctly generated, since for it we need to have the proper OS setup (user running the tests has to belong the a group used for these tests, and that obviously changes with every computer). What would you suggest?

[boegel]

this is not correct:

• uses non-existing variable ec_group
• string templating with a tuple value (group_spec) doesn't work for a single %s, should be str(group_spec)

fixed in damianam#11, which includes an enhanced test

[boegel]

Going in, thanks @damianam!

[mboisson]

Can we make this optional through a config item ? That PR caused a bug in our installed recipes.

[boegel]

@mboisson Sure we can. Any particular reason you don't like to have this enabled by default? And are you up for looking into the PR to make this optional yourself? ;)

[mboisson]

We use other means of managing user group checks, and the code added by this PR is broken on our system.

[mboisson]

Specifically, we do want easybuild to set the correct group-owner for the files, but we do not want to have the check in the module.

[bartoldeman]

I'll work on a PR. The idea could be that
group = ("supergroup", None)
can do what we want.

[boegel]

@bartoldeman Thanks, but, your idea is not going to work out well, since if None is specified as error message, EasyBuild falls back to a standard error message for users not in the group, see https://github.com/easybuilders/easybuild-framework/pull/2274/files#diff-8cdd2ad54096882655bd836265e8e26bR677.

A configuration option like --add-user-in-group-check (that is enabled by default, so you could use --disable-add-user-in-group-check) seems like a better option?

[damianam]

We use other means of managing user group checks, and the code added by this PR is broken on our system.

Broken how exactly? We definitely don't want to have broken code anywhere, if it breaks in your system I guess it is a bug on the current implementation.

Specifically, we do want easybuild to set the correct group-owner for the files, but we do not want to have the check in the module.

Can I ask why? I have no problem with adding options to remove this check (even though I can't spend time on it until November the earliest), but I wonder what is the rationale behind. Understanding the issue might help to have a better solution.

[mboisson]

Broken how: For some reason, on our system, the "userInGroup" function from Lmod fails to resolve the group name, and barfs on numeric IDs. We have a "localUserInGroup" function which works. It does not

local function localUserInGroup(group)
        local handle = io.popen("groups 2>/dev/null")
        local grps = handle:read()
        handle:close()
        local found  = false
        for g in string.gmatch(grps, '([^ ]+)') do
                if (g == group)  then
                        found = true
                        break
                end
        end
        return found
end

In comparison, Lmod's userInGroup does:

function userInGroup(group)
   local grps   = capture("groups")
   local found  = false
   local userId = capture("id -u")
   local isRoot = tonumber(userId) == 0
   for g in grps:split("[ \n]") do
      if (g == group or isRoot)  then
         found = true
         break
      end
   end
   return found
end

We also tend to centralize any such check in the SitePackage.lua, through a hook. This prevents having one version of a software which has the check, and a second version which does not.

[bartoldeman]

Not a PR yet but at least I solved the mystery on why this happened:
according to http://lmod.readthedocs.io/en/latest/050_lua_modulefiles.html

capture (“string”):
Run the “string” as a command and capture the output. This function uses the value of LD_PRELOAD and LD_LIBRARY_PATH found when Lmod is configured. Use subprocess if you wish to use the current values.

A problem with our Nix setup is that we rely on LD_LIBRARY_PATH to find libnss_sss.so.2 to authenticate successfully. Since the LD_LIBRARY_PATH was not set when Lmod was configured, the capture command did not get the group names. I have now fixed our Lmod to avoid this, but in any case Maxime's comments still hold (the message overrides our logic in SitePackage.lua which is as follows):

                                if (not localUserInGroup(groupT[name])) then
                                        log_module_load(t,false)
                                        local message_found = false
                                        for k2,v2 in pairs(posix_group_messageT) do
                                                if (has_value(k2,name)) then
                                                        LmodError(v2)
                                                        message_found = true
                                                end
                                        end
                                        if (not message_found) then
                                                LmodError(posix_group_message)
                                        end
                                end

if the module errors out on its own, log_module_load(t,false) fails and the LmodError in the above chunk is never invoked.

[mboisson]

How about

group = ("supergroup", "")

(rather than the above suggestion of "None") can do what we want ?