Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit2f54f76

Browse files
authored
feat: allow IDP to return single string for roles/groups claim (#10993)
* feat: allow IDP to return single string instead of array for roles/groups claimThis is to support ADFS
1 parent3883d71 commit2f54f76

File tree

3 files changed

+259
-39
lines changed

3 files changed

+259
-39
lines changed

‎coderd/userauth.go

Lines changed: 71 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1019,31 +1019,26 @@ func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interfac
10191019
ifapi.OIDCConfig.GroupField!="" {
10201020
usingGroups=true
10211021
groupsRaw,ok:=mergedClaims[api.OIDCConfig.GroupField]
1022-
ifok&&api.OIDCConfig.GroupField!="" {
1023-
// Convert the []interface{} we get to a []string.
1024-
groupsInterface,ok:=groupsRaw.([]interface{})
1025-
ifok {
1026-
api.Logger.Debug(ctx,"groups returned in oidc claims",
1027-
slog.F("len",len(groupsInterface)),
1028-
slog.F("groups",groupsInterface),
1022+
ifok {
1023+
parsedGroups,err:=parseStringSliceClaim(groupsRaw)
1024+
iferr!=nil {
1025+
api.Logger.Debug(ctx,"groups field was an unknown type in oidc claims",
1026+
slog.F("type",fmt.Sprintf("%T",groupsRaw)),
1027+
slog.Error(err),
10291028
)
1029+
returnfalse,nil,err
1030+
}
10301031

1031-
for_,groupInterface:=rangegroupsInterface {
1032-
group,ok:=groupInterface.(string)
1033-
if!ok {
1034-
returnfalse,nil,xerrors.Errorf("Invalid group type. Expected string, got: %T",groupInterface)
1035-
}
1036-
1037-
ifmappedGroup,ok:=api.OIDCConfig.GroupMapping[group];ok {
1038-
group=mappedGroup
1039-
}
1032+
api.Logger.Debug(ctx,"groups returned in oidc claims",
1033+
slog.F("len",len(parsedGroups)),
1034+
slog.F("groups",parsedGroups),
1035+
)
10401036

1041-
groups=append(groups,group)
1037+
for_,group:=rangeparsedGroups {
1038+
ifmappedGroup,ok:=api.OIDCConfig.GroupMapping[group];ok {
1039+
group=mappedGroup
10421040
}
1043-
}else {
1044-
api.Logger.Debug(ctx,"groups field was an unknown type",
1045-
slog.F("type",fmt.Sprintf("%T",groupsRaw)),
1046-
)
1041+
groups=append(groups,group)
10471042
}
10481043
}
10491044
}
@@ -1079,10 +1074,11 @@ func (api *API) oidcRoles(ctx context.Context, rw http.ResponseWriter, r *http.R
10791074
rolesRow= []interface{}{}
10801075
}
10811076

1082-
rolesInterface,ok:=rolesRow.([]interface{})
1083-
if!ok {
1084-
api.Logger.Error(ctx,"oidcclaim user roles field was an unknown type",
1077+
parsedRoles,err:=parseStringSliceClaim(rolesRow)
1078+
iferr!=nil {
1079+
api.Logger.Error(ctx,"oidcclaims user roles field was an unknown type",
10851080
slog.F("type",fmt.Sprintf("%T",rolesRow)),
1081+
slog.Error(err),
10861082
)
10871083
site.RenderStaticErrorPage(rw,r, site.ErrorPageData{
10881084
Status:http.StatusInternalServerError,
@@ -1096,21 +1092,10 @@ func (api *API) oidcRoles(ctx context.Context, rw http.ResponseWriter, r *http.R
10961092
}
10971093

10981094
api.Logger.Debug(ctx,"roles returned in oidc claims",
1099-
slog.F("len",len(rolesInterface)),
1100-
slog.F("roles",rolesInterface),
1095+
slog.F("len",len(parsedRoles)),
1096+
slog.F("roles",parsedRoles),
11011097
)
1102-
for_,roleInterface:=rangerolesInterface {
1103-
role,ok:=roleInterface.(string)
1104-
if!ok {
1105-
api.Logger.Error(ctx,"invalid oidc user role type",
1106-
slog.F("type",fmt.Sprintf("%T",rolesRow)),
1107-
)
1108-
httpapi.Write(ctx,rw,http.StatusBadRequest, codersdk.Response{
1109-
Message:fmt.Sprintf("Invalid user role type. Expected string, got: %T",roleInterface),
1110-
})
1111-
returnnil,false
1112-
}
1113-
1098+
for_,role:=rangeparsedRoles {
11141099
ifmappedRoles,ok:=api.OIDCConfig.UserRoleMapping[role];ok {
11151100
iflen(mappedRoles)==0 {
11161101
continue
@@ -1449,7 +1434,7 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
14491434
iferr!=nil {
14501435
returnhttpError{
14511436
code:http.StatusBadRequest,
1452-
msg:"Invalid roles through OIDCclaim",
1437+
msg:"Invalid roles through OIDCclaims",
14531438
detail:fmt.Sprintf("Error from role assignment attempt: %s",err.Error()),
14541439
renderStaticPage:true,
14551440
}
@@ -1744,3 +1729,50 @@ func wrongLoginTypeHTTPError(user database.LoginType, params database.LoginType)
17441729
params,user,addedMsg),
17451730
}
17461731
}
1732+
1733+
// parseStringSliceClaim parses the claim for groups and roles, expected []string.
1734+
//
1735+
// Some providers like ADFS return a single string instead of an array if there
1736+
// is only 1 element. So this function handles the edge cases.
1737+
funcparseStringSliceClaim(claiminterface{}) ([]string,error) {
1738+
groups:=make([]string,0)
1739+
ifclaim==nil {
1740+
returngroups,nil
1741+
}
1742+
1743+
// The simple case is the type is exactly what we expected
1744+
asStringArray,ok:=claim.([]string)
1745+
ifok {
1746+
returnasStringArray,nil
1747+
}
1748+
1749+
asArray,ok:=claim.([]interface{})
1750+
ifok {
1751+
fori,item:=rangeasArray {
1752+
asString,ok:=item.(string)
1753+
if!ok {
1754+
returnnil,xerrors.Errorf("invalid claim type. Element %d expected a string, got: %T",i,item)
1755+
}
1756+
groups=append(groups,asString)
1757+
}
1758+
returngroups,nil
1759+
}
1760+
1761+
asString,ok:=claim.(string)
1762+
ifok {
1763+
ifasString=="" {
1764+
// Empty string should be 0 groups.
1765+
return []string{},nil
1766+
}
1767+
// If it is a single string, first check if it is a csv.
1768+
// If a user hits this, it is likely a misconfiguration and they need
1769+
// to reconfigure their IDP to send an array instead.
1770+
ifstrings.Contains(asString,",") {
1771+
returnnil,xerrors.Errorf("invalid claim type. Got a csv string (%q), change this claim to return an array of strings instead.",asString)
1772+
}
1773+
return []string{asString},nil
1774+
}
1775+
1776+
// Not sure what the user gave us.
1777+
returnnil,xerrors.Errorf("invalid claim type. Expected an array of strings, got: %T",claim)
1778+
}

‎coderd/userauth_internal_test.go

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
package coderd
2+
3+
import (
4+
"encoding/json"
5+
"testing"
6+
7+
"github.com/stretchr/testify/require"
8+
)
9+
10+
funcTestParseStringSliceClaim(t*testing.T) {
11+
t.Parallel()
12+
13+
cases:= []struct {
14+
Namestring
15+
GoClaiminterface{}
16+
// JSON Claim allows testing the json -> go conversion
17+
// of some strings.
18+
JSONClaimstring
19+
ErrorExpectedbool
20+
ExpectedSlice []string
21+
}{
22+
{
23+
Name:"Nil",
24+
GoClaim:nil,
25+
ExpectedSlice: []string{},
26+
},
27+
// Go Slices
28+
{
29+
Name:"EmptySlice",
30+
GoClaim: []string{},
31+
ExpectedSlice: []string{},
32+
},
33+
{
34+
Name:"StringSlice",
35+
GoClaim: []string{"a","b","c"},
36+
ExpectedSlice: []string{"a","b","c"},
37+
},
38+
{
39+
Name:"InterfaceSlice",
40+
GoClaim: []interface{}{"a","b","c"},
41+
ExpectedSlice: []string{"a","b","c"},
42+
},
43+
{
44+
Name:"MixedSlice",
45+
GoClaim: []interface{}{"a",string("b"),interface{}("c")},
46+
ExpectedSlice: []string{"a","b","c"},
47+
},
48+
{
49+
Name:"StringSliceOneElement",
50+
GoClaim: []string{"a"},
51+
ExpectedSlice: []string{"a"},
52+
},
53+
// Json Slices
54+
{
55+
Name:"JSONEmptySlice",
56+
JSONClaim:`[]`,
57+
ExpectedSlice: []string{},
58+
},
59+
{
60+
Name:"JSONStringSlice",
61+
JSONClaim:`["a", "b", "c"]`,
62+
ExpectedSlice: []string{"a","b","c"},
63+
},
64+
{
65+
Name:"JSONStringSliceOneElement",
66+
JSONClaim:`["a"]`,
67+
ExpectedSlice: []string{"a"},
68+
},
69+
// Go string
70+
{
71+
Name:"String",
72+
GoClaim:"a",
73+
ExpectedSlice: []string{"a"},
74+
},
75+
{
76+
Name:"EmptyString",
77+
GoClaim:"",
78+
ExpectedSlice: []string{},
79+
},
80+
{
81+
Name:"Interface",
82+
GoClaim:interface{}("a"),
83+
ExpectedSlice: []string{"a"},
84+
},
85+
// JSON string
86+
{
87+
Name:"JSONString",
88+
JSONClaim:`"a"`,
89+
ExpectedSlice: []string{"a"},
90+
},
91+
{
92+
Name:"JSONEmptyString",
93+
JSONClaim:`""`,
94+
ExpectedSlice: []string{},
95+
},
96+
// Go Errors
97+
{
98+
Name:"IntegerInSlice",
99+
GoClaim: []interface{}{"a","b",1},
100+
ErrorExpected:true,
101+
},
102+
// Json Errors
103+
{
104+
Name:"JSONIntegerInSlice",
105+
JSONClaim:`["a", "b", 1]`,
106+
ErrorExpected:true,
107+
},
108+
{
109+
Name:"JSON_CSV",
110+
JSONClaim:`"a,b,c"`,
111+
ErrorExpected:true,
112+
},
113+
}
114+
115+
for_,c:=rangecases {
116+
c:=c
117+
t.Run(c.Name,func(t*testing.T) {
118+
t.Parallel()
119+
120+
iflen(c.JSONClaim)>0 {
121+
require.Nil(t,c.GoClaim,"go claim should be nil if json set")
122+
err:=json.Unmarshal([]byte(c.JSONClaim),&c.GoClaim)
123+
require.NoError(t,err,"unmarshal json claim")
124+
}
125+
126+
found,err:=parseStringSliceClaim(c.GoClaim)
127+
ifc.ErrorExpected {
128+
require.Error(t,err)
129+
}else {
130+
require.NoError(t,err)
131+
require.ElementsMatch(t,c.ExpectedSlice,found,"expected groups")
132+
}
133+
})
134+
}
135+
}

‎enterprise/coderd/userauth_test.go

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,33 @@ func TestUserOIDC(t *testing.T) {
5555
runner.AssertRoles(t,"alice", []string{})
5656
})
5757

58+
// Some IDPs (ADFS) send the "string" type vs "[]string" if only
59+
// 1 role exists.
60+
t.Run("SingleRoleString",func(t*testing.T) {
61+
t.Parallel()
62+
63+
constoidcRoleName="TemplateAuthor"
64+
runner:=setupOIDCTest(t,oidcTestConfig{
65+
Config:func(cfg*coderd.OIDCConfig) {
66+
cfg.AllowSignups=true
67+
cfg.UserRoleField="roles"
68+
cfg.UserRoleMapping=map[string][]string{
69+
oidcRoleName: {rbac.RoleTemplateAdmin()},
70+
}
71+
},
72+
})
73+
74+
// User starts with the owner role
75+
_,resp:=runner.Login(t, jwt.MapClaims{
76+
"email":"alice@coder.com",
77+
// This is sent as a **string** intentionally instead
78+
// of an array.
79+
"roles":oidcRoleName,
80+
})
81+
require.Equal(t,http.StatusOK,resp.StatusCode)
82+
runner.AssertRoles(t,"alice", []string{rbac.RoleTemplateAdmin()})
83+
})
84+
5885
// A user has some roles, then on an oauth refresh will lose said
5986
// roles from an updated claim.
6087
t.Run("NewUserAndRemoveRolesOnRefresh",func(t*testing.T) {
@@ -334,6 +361,32 @@ func TestUserOIDC(t *testing.T) {
334361
require.Equal(t,http.StatusOK,resp.StatusCode)
335362
runner.AssertGroups(t,"alice", []string{groupName})
336363
})
364+
365+
// Some IDPs (ADFS) send the "string" type vs "[]string" if only
366+
// 1 group exists.
367+
t.Run("SingleRoleGroup",func(t*testing.T) {
368+
t.Parallel()
369+
370+
constgroupClaim="custom-groups"
371+
constgroupName="bingbong"
372+
runner:=setupOIDCTest(t,oidcTestConfig{
373+
Config:func(cfg*coderd.OIDCConfig) {
374+
cfg.AllowSignups=true
375+
cfg.GroupField=groupClaim
376+
cfg.CreateMissingGroups=true
377+
},
378+
})
379+
380+
// User starts with the owner role
381+
_,resp:=runner.Login(t, jwt.MapClaims{
382+
"email":"alice@coder.com",
383+
// This is sent as a **string** intentionally instead
384+
// of an array.
385+
groupClaim:groupName,
386+
})
387+
require.Equal(t,http.StatusOK,resp.StatusCode)
388+
runner.AssertGroups(t,"alice", []string{groupName})
389+
})
337390
})
338391

339392
t.Run("Refresh",func(t*testing.T) {

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp