diff --git a/coderd/idpsync/group.go b/coderd/idpsync/group.go index 0b21c5b9ac84c..63ac0360f0cb3 100644 --- a/coderd/idpsync/group.go +++ b/coderd/idpsync/group.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "net/http" "github.com/golang-jwt/jwt/v4" "github.com/google/uuid" @@ -71,9 +72,49 @@ func (s AGPLIDPSync) GroupSyncSettings(ctx context.Context, orgID uuid.UUID, db return settings, nil } -func (s AGPLIDPSync) ParseGroupClaims(_ context.Context, _ jwt.MapClaims) (GroupParams, *HTTPError) { +func (s AGPLIDPSync) ParseGroupClaims(_ context.Context, mergedClaims jwt.MapClaims) (GroupParams, *HTTPError) { + if s.GroupField != "" && len(s.GroupAllowList) > 0 { + groupsRaw, ok := mergedClaims[s.GroupField] + if !ok { + return GroupParams{}, &HTTPError{ + Code: http.StatusForbidden, + Msg: "Not a member of an allowed group", + Detail: "You have no groups in your claims!", + RenderStaticPage: true, + } + } + parsedGroups, err := ParseStringSliceClaim(groupsRaw) + if err != nil { + return GroupParams{}, &HTTPError{ + Code: http.StatusBadRequest, + Msg: "Failed read groups from claims for allow list check. Ask an administrator for help.", + Detail: err.Error(), + RenderStaticPage: true, + } + } + + inAllowList := false + AllowListCheckLoop: + for _, group := range parsedGroups { + if _, ok := s.GroupAllowList[group]; ok { + inAllowList = true + break AllowListCheckLoop + } + } + + if !inAllowList { + return GroupParams{}, &HTTPError{ + Code: http.StatusForbidden, + Msg: "Not a member of an allowed group", + Detail: "Ask an administrator to add one of your groups to the allow list.", + RenderStaticPage: true, + } + } + } + return GroupParams{ SyncEntitled: s.GroupSyncEntitled(), + MergedClaims: mergedClaims, }, nil } diff --git a/coderd/idpsync/group_test.go b/coderd/idpsync/group_test.go index 478d6557de551..81320053c5ba1 100644 --- a/coderd/idpsync/group_test.go +++ b/coderd/idpsync/group_test.go @@ -44,8 +44,7 @@ func TestParseGroupClaims(t *testing.T) { require.False(t, params.SyncEntitled) }) - // AllowList has no effect in AGPL - t.Run("AllowList", func(t *testing.T) { + t.Run("NotInAllowList", func(t *testing.T) { t.Parallel() s := idpsync.NewAGPLSync(slogtest.Make(t, &slogtest.Options{}), @@ -59,9 +58,39 @@ func TestParseGroupClaims(t *testing.T) { ctx := testutil.Context(t, testutil.WaitMedium) - params, err := s.ParseGroupClaims(ctx, jwt.MapClaims{}) + // Invalid group + _, err := s.ParseGroupClaims(ctx, jwt.MapClaims{ + "groups": []string{"bar"}, + }) + require.NotNil(t, err) + require.Equal(t, 403, err.Code) + + // No groups + _, err = s.ParseGroupClaims(ctx, jwt.MapClaims{}) + require.NotNil(t, err) + require.Equal(t, 403, err.Code) + }) + + t.Run("InAllowList", func(t *testing.T) { + t.Parallel() + + s := idpsync.NewAGPLSync(slogtest.Make(t, &slogtest.Options{}), + runtimeconfig.NewManager(), + idpsync.DeploymentSyncSettings{ + GroupField: "groups", + GroupAllowList: map[string]struct{}{ + "foo": {}, + }, + }) + + ctx := testutil.Context(t, testutil.WaitMedium) + + claims := jwt.MapClaims{ + "groups": []string{"foo", "bar"}, + } + params, err := s.ParseGroupClaims(ctx, claims) require.Nil(t, err) - require.False(t, params.SyncEntitled) + require.Equal(t, claims, params.MergedClaims) }) } diff --git a/enterprise/coderd/enidpsync/groups.go b/enterprise/coderd/enidpsync/groups.go index 7cabce412a1ea..c67d8d53f0501 100644 --- a/enterprise/coderd/enidpsync/groups.go +++ b/enterprise/coderd/enidpsync/groups.go @@ -2,7 +2,6 @@ package enidpsync import ( "context" - "net/http" "github.com/golang-jwt/jwt/v4" @@ -20,51 +19,12 @@ func (e EnterpriseIDPSync) GroupSyncEntitled() bool { // GroupAllowList is implemented here to prevent login by unauthorized users. // TODO: GroupAllowList overlaps with the default organization group sync settings. func (e EnterpriseIDPSync) ParseGroupClaims(ctx context.Context, mergedClaims jwt.MapClaims) (idpsync.GroupParams, *idpsync.HTTPError) { - if !e.GroupSyncEntitled() { - return e.AGPLIDPSync.ParseGroupClaims(ctx, mergedClaims) + resp, err := e.AGPLIDPSync.ParseGroupClaims(ctx, mergedClaims) + if err != nil { + return idpsync.GroupParams{}, err } - - if e.GroupField != "" && len(e.GroupAllowList) > 0 { - groupsRaw, ok := mergedClaims[e.GroupField] - if !ok { - return idpsync.GroupParams{}, &idpsync.HTTPError{ - Code: http.StatusForbidden, - Msg: "Not a member of an allowed group", - Detail: "You have no groups in your claims!", - RenderStaticPage: true, - } - } - parsedGroups, err := idpsync.ParseStringSliceClaim(groupsRaw) - if err != nil { - return idpsync.GroupParams{}, &idpsync.HTTPError{ - Code: http.StatusBadRequest, - Msg: "Failed read groups from claims for allow list check. Ask an administrator for help.", - Detail: err.Error(), - RenderStaticPage: true, - } - } - - inAllowList := false - AllowListCheckLoop: - for _, group := range parsedGroups { - if _, ok := e.GroupAllowList[group]; ok { - inAllowList = true - break AllowListCheckLoop - } - } - - if !inAllowList { - return idpsync.GroupParams{}, &idpsync.HTTPError{ - Code: http.StatusForbidden, - Msg: "Not a member of an allowed group", - Detail: "Ask an administrator to add one of your groups to the allow list.", - RenderStaticPage: true, - } - } - } - return idpsync.GroupParams{ - SyncEntitled: true, - MergedClaims: mergedClaims, + SyncEntitled: e.GroupSyncEntitled(), + MergedClaims: resp.MergedClaims, }, nil }