From 633e51369731903d566349e3cb548f5576821069 Mon Sep 17 00:00:00 2001 From: James Read Date: Tue, 22 Apr 2025 14:35:49 +0100 Subject: [PATCH] feature: (#568) Separator allowed in usergroup line for trusted headers (#572) --- service/internal/acl/acl.go | 66 ++++++++----- service/internal/acl/acl_test.go | 110 +++++++++++++++++---- service/internal/config/config.go | 1 + service/internal/executor/arguments.go | 2 +- service/internal/executor/executor_test.go | 12 +-- service/internal/grpcapi/grpcApi.go | 6 +- 6 files changed, 145 insertions(+), 52 deletions(-) diff --git a/service/internal/acl/acl.go b/service/internal/acl/acl.go index e91b9f8..bcf5ea0 100644 --- a/service/internal/acl/acl.go +++ b/service/internal/acl/acl.go @@ -25,8 +25,8 @@ func (p PermissionBits) Has(permission PermissionBits) bool { // User respresents a person. type AuthenticatedUser struct { - Username string - Usergroup string + Username string + UsergroupLine string Provider string SID string @@ -40,6 +40,36 @@ func (u *AuthenticatedUser) IsGuest() bool { return u.Username == "guest" && u.Provider == "system" } +func (u *AuthenticatedUser) parseUsergroupLine(sep string) []string { + ret := []string{} + + if sep != "" { + for _, v := range strings.Split(u.UsergroupLine, sep) { + trimmed := strings.TrimSpace(v) + + if trimmed != "" { + ret = append(ret, trimmed) + } + } + } else { + ret = strings.Fields(u.UsergroupLine) + } + return ret +} + +func (u *AuthenticatedUser) matchesUsergroupAcl(matchUsergroups []string, sep string) bool { + groupList := u.parseUsergroupLine(sep) + + for _, group := range groupList { + if slices.Contains(matchUsergroups, group) { + log.Debugf("Usergroup %v found in %+v (len: %v)", group, groupList, len(groupList)) + return true + } + } + + return false +} + func logAclNotMatched(cfg *config.Config, aclFunction string, user *AuthenticatedUser, action *config.Action, acl *config.AccessControlList) { if cfg.LogDebugOptions.AclNotMatched { log.WithFields(log.Fields{ @@ -101,7 +131,7 @@ func aclCheck(requiredPermission PermissionBits, defaultValue bool, cfg *config. log.WithFields(log.Fields{ "actionTitle": action.Title, "username": user.Username, - "usergroup": user.Usergroup, + "usergroupLine": user.UsergroupLine, "relevantAcls": len(relevantAcls), "requiredPermission": requiredPermission, }).Debugf("ACL check - %v", aclFunction) @@ -162,7 +192,7 @@ func UserFromContext(ctx context.Context, cfg *config.Config) *AuthenticatedUser if ok { ret = &AuthenticatedUser{} ret.Username = getMetadataKeyOrEmpty(md, "username") - ret.Usergroup = getMetadataKeyOrEmpty(md, "usergroup") + ret.UsergroupLine = getMetadataKeyOrEmpty(md, "usergroup") ret.Provider = getMetadataKeyOrEmpty(md, "provider") buildUserAcls(cfg, ret) @@ -173,10 +203,10 @@ func UserFromContext(ctx context.Context, cfg *config.Config) *AuthenticatedUser } log.WithFields(log.Fields{ - "username": ret.Username, - "usergroup": ret.Usergroup, - "provider": ret.Provider, - "acls": ret.Acls, + "username": ret.Username, + "usergroupLine": ret.UsergroupLine, + "provider": ret.Provider, + "acls": ret.Acls, }).Debugf("UserFromContext") return ret @@ -185,7 +215,7 @@ func UserFromContext(ctx context.Context, cfg *config.Config) *AuthenticatedUser func UserGuest(cfg *config.Config) *AuthenticatedUser { ret := &AuthenticatedUser{} ret.Username = "guest" - ret.Usergroup = "guest" + ret.UsergroupLine = "guest" ret.Provider = "system" buildUserAcls(cfg, ret) @@ -195,9 +225,9 @@ func UserGuest(cfg *config.Config) *AuthenticatedUser { func UserFromSystem(cfg *config.Config, username string) *AuthenticatedUser { ret := &AuthenticatedUser{ - Username: username, - Usergroup: "system", - Provider: "system", + Username: username, + UsergroupLine: "system", + Provider: "system", } buildUserAcls(cfg, ret) @@ -212,8 +242,7 @@ func buildUserAcls(cfg *config.Config, user *AuthenticatedUser) { continue } - // handle multiple usergroups - groups will be separated by a space - if hasGroupsMatch(acl.MatchUsergroups, user.Usergroup) { + if user.matchesUsergroupAcl(acl.MatchUsergroups, cfg.AuthHttpHeaderUserGroupSep) { user.Acls = append(user.Acls, acl.Name) continue } @@ -222,15 +251,6 @@ func buildUserAcls(cfg *config.Config, user *AuthenticatedUser) { user.EffectivePolicy = getEffectivePolicy(cfg, user) } -func hasGroupsMatch(matchUsergroups []string, usergroup string) bool { - for _, group := range strings.Fields(usergroup) { - if slices.Contains(matchUsergroups, group) { - return true - } - } - return false -} - func isACLRelevantToAction(cfg *config.Config, actionAcls []string, acl *config.AccessControlList, user *AuthenticatedUser) bool { if !slices.Contains(user.Acls, acl.Name) { // If the user does not have this ACL, then it is not relevant diff --git a/service/internal/acl/acl_test.go b/service/internal/acl/acl_test.go index 58f36de..48e4992 100644 --- a/service/internal/acl/acl_test.go +++ b/service/internal/acl/acl_test.go @@ -1,37 +1,111 @@ package acl -import "testing" +import ( + "github.com/stretchr/testify/assert" + "testing" +) func Test_hasGroupsMatch(t *testing.T) { tests := []struct { - name string - matchUsergroups []string - usergroup string - want bool + name string + aclMatchUsergroups []string + usergroupLine string + matches bool + sep string }{ { - name: "No groups match", - matchUsergroups: []string{"group1", "group2"}, - usergroup: "group3", + name: "No groups match", + aclMatchUsergroups: []string{"group1", "group2"}, + usergroupLine: "group3", + matches: false, }, { - name: "Exact match", - matchUsergroups: []string{"group1", "group2"}, - usergroup: "group1", - want: true, + name: "Exact match", + aclMatchUsergroups: []string{"group1", "group2"}, + usergroupLine: "group1", + matches: true, }, { - name: "Multiple groups match", - matchUsergroups: []string{"group1", "group2"}, - usergroup: "group1 group2", - want: true, + name: "Multiple groups match", + aclMatchUsergroups: []string{"group1", "group2"}, + usergroupLine: "group1 group2", + matches: true, + }, + { + name: "Comma-separated groups match", + aclMatchUsergroups: []string{"group1", "group2", "group3"}, + usergroupLine: "group1, group2", + matches: true, + sep: ",", + }, + { + name: "Comma-separated groups with default separator does not match", + aclMatchUsergroups: []string{"group1"}, + usergroupLine: "group1, group2", + matches: false, + sep: "", }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := hasGroupsMatch(tt.matchUsergroups, tt.usergroup); got != tt.want { - t.Errorf("hasGroupsMatch() = %v, want %v", got, tt.want) + user := &AuthenticatedUser{ + Username: "testuser", + UsergroupLine: tt.usergroupLine, + } + + if matches := user.matchesUsergroupAcl(tt.aclMatchUsergroups, tt.sep); matches != tt.matches { + t.Errorf("AuthenticatedUser.matchesUsergroupAcl() = %v, want %v for usergroups %v", matches, tt.matches, tt.aclMatchUsergroups) } }) } } + +func Test_parseUsergroupLine(t *testing.T) { + tests := []struct { + name string + usergroupLine string + expectedGroups []string + sep string + }{ + { + name: "Default separator (space)", + usergroupLine: "group1 group2", + expectedGroups: []string{"group1", "group2"}, + }, + { + name: "Comma-separated groups", + usergroupLine: "group1 , group2", + expectedGroups: []string{"group1", "group2"}, + sep: ",", + }, + { + name: "Multiple spaces", + usergroupLine: "group1 , group2 , group3", + expectedGroups: []string{"group1", "group2", "group3"}, + sep: ",", + }, + { + name: "Empty usergroup line", + usergroupLine: "", + expectedGroups: []string{}, + }, + { + name: "Empty group names", + usergroupLine: "|group1| | group3|", + expectedGroups: []string{"group1", "group3"}, + sep: "|", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + user := &AuthenticatedUser{ + Username: "testuser", + UsergroupLine: tt.usergroupLine, + } + + assert.Equal(t, tt.expectedGroups, user.parseUsergroupLine(tt.sep)) + }) + } +} diff --git a/service/internal/config/config.go b/service/internal/config/config.go index ad1c818..fadb024 100644 --- a/service/internal/config/config.go +++ b/service/internal/config/config.go @@ -124,6 +124,7 @@ type Config struct { AuthJwtPubKeyPath string // will read pub key from file on disk AuthHttpHeaderUsername string AuthHttpHeaderUserGroup string + AuthHttpHeaderUserGroupSep string AuthLocalUsers AuthLocalUsersConfig AuthLoginUrl string AuthRequireGuestsToLogin bool diff --git a/service/internal/executor/arguments.go b/service/internal/executor/arguments.go index edc94d8..df7770d 100644 --- a/service/internal/executor/arguments.go +++ b/service/internal/executor/arguments.go @@ -6,12 +6,12 @@ import ( log "github.com/sirupsen/logrus" "errors" + "fmt" "net/mail" "net/url" "regexp" "strings" "time" - "fmt" ) var ( diff --git a/service/internal/executor/executor_test.go b/service/internal/executor/executor_test.go index dd873c9..18863f1 100644 --- a/service/internal/executor/executor_test.go +++ b/service/internal/executor/executor_test.go @@ -186,8 +186,8 @@ func TestUnsetRequiredArgument(t *testing.T) { Shell: "echo 'Your name is: {{ name }}'", Arguments: []config.ActionArgument{ { - Name: "name", - Type: "ascii", + Name: "name", + Type: "ascii", }, }, } @@ -206,12 +206,12 @@ func TestUnusedArgumentStillPassesTypeSafetyCheck(t *testing.T) { Shell: "echo 'Your name is: {{ name }}'", Arguments: []config.ActionArgument{ { - Name: "name", - Type: "ascii", + Name: "name", + Type: "ascii", }, { - Name: "age", - Type: "int", + Name: "age", + Type: "int", }, }, } diff --git a/service/internal/grpcapi/grpcApi.go b/service/internal/grpcapi/grpcApi.go index d7424b3..007f64f 100644 --- a/service/internal/grpcapi/grpcApi.go +++ b/service/internal/grpcapi/grpcApi.go @@ -316,7 +316,7 @@ func (api *oliveTinAPI) GetDashboardComponents(ctx ctx.Context, req *apiv1.GetDa if len(res.Actions) == 0 { log.WithFields(log.Fields{ "username": user.Username, - "usergroup": user.Usergroup, + "usergroupLine": user.UsergroupLine, "provider": user.Provider, "acls": user.Acls, "availableActions": len(cfg.Actions), @@ -377,14 +377,12 @@ func (api *oliveTinAPI) WhoAmI(ctx ctx.Context, req *apiv1.WhoAmIRequest) (*apiv res := &apiv1.WhoAmIResponse{ AuthenticatedUser: user.Username, - Usergroup: user.Usergroup, + Usergroup: user.UsergroupLine, Provider: user.Provider, Sid: user.SID, Acls: user.Acls, } - log.Warnf("usergroup: %v", user.Usergroup) - return res, nil }