feat: add local groups support #122
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
didrocks
force-pushed
the
add_system_groups
branch
from
ec9a6d1 to
14d905f
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #122 +/- ##
==========================================
+ Coverage 87.36% 87.65% +0.28%
==========================================
Files 29 30 +1
Lines 2114 2212 +98
==========================================
+ Hits 1847 1939 +92
- Misses 205 209 +4
- Partials 62 64 +2 ☔ View full report in Codecov by Sentry. |
didrocks
force-pushed
the
add_system_groups
branch
from
14d905f to
aa3cd79
Compare
|
FYI, this created 2 more cards, for local group purge (on invalid cache and when user is purged) and also empty groups/empty data policy: |
didrocks
force-pushed
the
add_system_groups
branch
from
aa3cd79 to
d35a8a6
Compare
denisonbarbosa
requested changes
Nov 30, 2023
This is our way to communicate that this is a system/local group.
Those are now a pointer as they can be nil to signify this is az local group.
This test ensure that all groups without gid are filtered and not stored in the cache.
didrocks
force-pushed
the
add_system_groups
branch
from
d35a8a6 to
5a15229
Compare
|
and here we go! In addition to the fixes, as discussed, there are 2 changes I wanted to do:
|
This package will be used to add/remove users from local groups as this information is not part of the cache. Adapt imports and tests for the new struct location.
Parse local group files and sync current authd user groups with the list provided by the broker.
Add the user to sudo in the example broker to articulate the local goup functionality.
Add tests for local group supports. Mock gpasswd binary to achieve this as we can’t change the prefix but only can chroot which requires running tests as root.
Commit the golden files, being the gpasswd output when called with given arguments and not returning an error.
We will need it in other places for integration tests. Make it then exportable on its own package, using the linkname to change set global defaults as we don’t want to leak the With* tests helpers throughout the stack. However, the tests using those won’t be able to be run in parallel.
Allow the broker mock to return multiple groups, including local ones, still producing valid JSON. Those tests needs to reuse the helper to change the default and thus, we can’t run run those tests in parallel.
Now, we are going to not pre-create gpasswd output file but only create it once the mock is called. That allows us to separate it being called or not. This functionality will be better in testutils, but it needs to all post-treatment for the file to be idemnpotent and I didn't find an elegant way for it. Then, we can replace the LoadWithUpdateFromGolden with something treating files (or maybe take back the tree comparison).
This suppresses all gpasswd.output files for tests not calling it.
Do not rely on the file content itself, but on its existence.
didrocks
force-pushed
the
add_system_groups
branch
from
5a15229 to
4117d99
Compare
3v1n0
reviewed
Nov 30, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds local group support by adding and update the local group list so that any remotely created user can be part of local groups.
For this, the broker specificies groups with only the group name and no ugid. Then, the group is considered local and the user added to it. If the local group doesn’t exist, then the group is simply ignored (only an info is triggered).
We are using gpasswd which doesn’t enforce the user to be in
/etc/passwd.UDENG-1444