PAM service tests

[denisonbarbosa]

Package tests for internal/services/pam.

UDENG-1172

[GabrielNagy]

Noticed some irregularities related to the pesky golden files, otherwise my comments are all minor :).

Very nice work and thanks for separating into individual commits!

[didrocks]

Congrats on the amount of tests and being able to still run them in parallel! This will be really helpful in the long run.
I think you can even easily aim for 100% coverage by covering the last 2 lines with a single test: passing an invalid layout capability as a request (missing "type")!

I think there is still some work to be done, in addition to quite some comments to enhance readability and maintainability. Here are three global remarks (which is repeated quite some times in this PR):

• we are using some conventions across multiple projects (fixtures directory, "-" to signify empty string in tests, no cmd/ import in internal/). Let’s stick with those please and don’t diverge from them. This helps then having consistency and familiarity across our work.
• my biggest issue is the cmd/ vs internal/ import. One consequence is that you then rely on the daemon to be created, with everything (manager, socket activation, and so on). You are basically building some integration tests here! The only thing you should import is internal/services/pam + everything that this one needs to be setup (apart from testutils, which is our test new requirements). This will avoid future potential loop imports and abstraction breakage.
• Similarly, some configuration in the fixtures are too specific. From this package, as long as the configuration is invalid, it’s invalid. We don’t want to be too much into details about missing this or that field. This is the problem of the brokers package which defines what is valid or invalid. If we change the validity rules later in the brokers package, it would force us to modify a lot of fixtures here. So, you should abstrarct to a higher level to avoid those future changes.

Finally, I think this is the right place to have two more complex tests to ensure we multiplex successfully:

• have at least 2 brokers
• select for user1 one broker
• select for user2 another broker
• give each one different layout capabilities
• ensure that each broker returns a different broker authentication mode
• calling is Authorized and ensure that both are acting independently.

I think this test can be for each broker in its own goroutine and only synchronize in the end that both are done.
This will allow, in particular with the race detector, to ensure we don’t have anything racy.

Then, rince and repeat in a second test with this time, two different users but with the same broker!

[3v1n0]

Since you're making various config paths to be tunable at code level, could you also please introduce arguments (or env variables) to define them when running authd manually (ideally supporting XDG or standard env variables when possible)?

i.e. for development purposes is convenient to have the ability to define temporary paths and use them to run the daemon as user instead of having to install it. Being also an exercise for special setups :)

[didrocks]

This is way better than the first proposal: way more readable and more inline with what we do regularly.

I have some nitpicks and a few enhancements proposal, nothing drastic and the core logic is here! Nice work :)

[didrocks]

Still some changes needed, but getting close.

[didrocks]

thanks for the changes! Good for merging