Shaman: fail unit test when running as root user (linux) #104234

MichaelC · 2023-07-06T04:35:24+02:00

MichaelC commented

2023-07-06 04:35:24 +02:00

If the mock tests are run by root user then this specific test of inaccessible path fails because root can write files to anywhere on the filesystem. It is not clear that Flamenco Manager test TestCheckSharedStoragePath is checking inaccessible file locations when it fails and that it should be run by an unprivileged user.

Fix is to fail the permission test if the tests are run as a root user.

Related: https://projects.blender.org/studio/flamenco/issues/104233 If the mock tests are run by root user then this specific test of inaccessible path fails because root can write files to anywhere on the filesystem. It is not clear that Flamenco Manager test TestCheckSharedStoragePath is checking inaccessible file locations when it fails and that it should be run by an unprivileged user. Fix is to fail the permission test if the tests are run as a root user.

MichaelC added 1 commit 2023-07-06 04:35:26 +02:00

only test non-root users have permission denied for inaccessible paths 4deef05cf9

MichaelC referenced this pull request

2023-07-06 04:35:42 +02:00

Flamenco Unit Test - meta_test.go - TestCheckSharedStoragePath fails if run as root user (linux) #104233

Sybren A. Stüvel referenced this pull request

2023-07-06 10:36:50 +02:00

Flamenco Unit Test - meta_test.go - TestCheckSharedStoragePath fails if run as root user (linux) #104233

Sybren A. Stüvel requested changes 2023-07-06 10:47:10 +02:00

Sybren A. Stüvel left a comment

Fix is to ignore this test if the user is root.

This IMO is not a good idea, especially because it happens silently. If this test is known to require a non-root account, it should simply fail if that requirement is not met.

> Fix is to ignore this test if the user is root. This IMO is not a good idea, especially because it happens silently. If this test is known to require a non-root account, it should simply fail if that requirement is not met.

internal/manager/api_impl/meta_test.go Outdated

						
				@ -198,1 +193,4 @@

						currentUser, err := user.Current()

						if err != nil {

							t.FailNow()

Sybren A. Stüvel commented

2023-07-06 10:46:26 +02:00

I feel there's too much nesting going on here. Also t.FailNow() wouldn't show the actual content of err. Finally, the original code was written before I realised I could do require.NoError(...) instead of assert.NoError(...) and it would immediately stop the test.

This would be better:

// Root can always create directories, so this test would fail with a
// confusing message. Instead it's better to refuse running as root at all.
currentUser, err := user.Current()
require.NoError(t, err)
require.NotEqual(t, "0", currentUser.Uid,
	"this test requires running as normal user, not %s (%s)", currentUser.Username, currentUser.Uid)
require.NotEqual(t, "root", currentUser.Username,
	"this test requires running as normal user, not %s (%s)", currentUser.Username, currentUser.Uid)

I feel there's too much nesting going on here. Also `t.FailNow()` wouldn't show the actual content of `err`. Finally, the original code was written before I realised I could do `require.NoError(...)` instead of `assert.NoError(...)` and it would immediately stop the test. This would be better: ```go // Root can always create directories, so this test would fail with a // confusing message. Instead it's better to refuse running as root at all. currentUser, err := user.Current() require.NoError(t, err) require.NotEqual(t, "0", currentUser.Uid, "this test requires running as normal user, not %s (%s)", currentUser.Username, currentUser.Uid) require.NotEqual(t, "root", currentUser.Username, "this test requires running as normal user, not %s (%s)", currentUser.Username, currentUser.Uid) ```

MichaelC added 1 commit 2023-07-06 14:59:54 +02:00

disallow root running the TestCheckSharedStoragePath test e3a6522af6

MichaelC commented

2023-07-06 15:00:29 +02:00

Thanks for the review. I agree with your points and failing on root is a better solution and using your code snippet makes sense.

Tested as root user inside a Docker container...

--- FAIL: TestCheckSharedStoragePath (0.00s)
    meta_test.go:198: 
        	Error Trace:	meta_test.go:198
        	Error:      	Should not be: "0"
        	Test:       	TestCheckSharedStoragePath
        	Messages:   	this test requires running as normal user, not root (0)

Thanks for the review. I agree with your points and failing on root is a better solution and using your code snippet makes sense. Tested as root user inside a Docker container... ``` --- FAIL: TestCheckSharedStoragePath (0.00s) meta_test.go:198: Error Trace: meta_test.go:198 Error: Should not be: "0" Test: TestCheckSharedStoragePath Messages: this test requires running as normal user, not root (0) ```

MichaelC requested review from Sybren A. Stüvel 2023-07-06 15:00:40 +02:00

MichaelC changed title from ~~Flamenco Unit Test - meta_test.go - TestCheckSharedStoragePath ignore permission test if tests are run as root user (linux)~~ to Flamenco Unit Test - meta_test.go - TestCheckSharedStoragePath fail permission test if tests are run as root user (linux)

2023-07-06 15:00:58 +02:00

Sybren A. Stüvel changed title from ~~Flamenco Unit Test - meta_test.go - TestCheckSharedStoragePath fail permission test if tests are run as root user (linux)~~ to Shaman: fail unit test when running as root user (linux)

2023-07-07 14:58:06 +02:00

Sybren A. Stüvel reviewed 2023-07-07 14:58:52 +02:00

internal/manager/api_impl/meta_test.go

						
				@ -203,6 +214,7 @@ func TestCheckSharedStoragePath(t *testing.T) {

						assert.Equal(t, testPath, result.Path)

						assert.False(t, result.IsUsable)

						assert.Contains(t, result.Cause, "Unable to create a file")