Skip to content

Commit cb6c0f3

Browse files
EmyrkEmyrk
authored
chore: refactor oidc group and role sync to methods (coder#10918)
The 'userOIDC' method body was getting unwieldy. I think there is a good way to redesign the flow, but I do not want to undertake that at this time. The easy win is just to move some LoC to other methods and cleanup the main method.
1 parent 2b71e38 commit cb6c0f3

File tree

1 file changed

+133
-103
lines changed

1 file changed

+133
-103
lines changed

‎coderd/userauth.go

Lines changed: 133 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -889,51 +889,6 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
889889
}
890890
}
891891

892-
var usingGroups bool
893-
var groups []string
894-
// If the GroupField is the empty string, then groups from OIDC are not used.
895-
// This is so we can support manual group assignment.
896-
if api.OIDCConfig.GroupField != "" {
897-
usingGroups = true
898-
groupsRaw, ok := mergedClaims[api.OIDCConfig.GroupField]
899-
if ok && api.OIDCConfig.GroupField != "" {
900-
// Convert the []interface{} we get to a []string.
901-
groupsInterface, ok := groupsRaw.([]interface{})
902-
if ok {
903-
api.Logger.Debug(ctx, "groups returned in oidc claims",
904-
slog.F("len", len(groupsInterface)),
905-
slog.F("groups", groupsInterface),
906-
)
907-
908-
for _, groupInterface := range groupsInterface {
909-
group, ok := groupInterface.(string)
910-
if !ok {
911-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
912-
Message: fmt.Sprintf("Invalid group type. Expected string, got: %T", groupInterface),
913-
})
914-
return
915-
}
916-
917-
if mappedGroup, ok := api.OIDCConfig.GroupMapping[group]; ok {
918-
group = mappedGroup
919-
}
920-
921-
groups = append(groups, group)
922-
}
923-
} else {
924-
api.Logger.Debug(ctx, "groups field was an unknown type",
925-
slog.F("type", fmt.Sprintf("%T", groupsRaw)),
926-
)
927-
}
928-
}
929-
}
930-
931-
// This conditional is purely to warn the user they might have misconfigured their OIDC
932-
// configuration.
933-
if _, groupClaimExists := mergedClaims["groups"]; !usingGroups && groupClaimExists {
934-
logger.Debug(ctx, "claim 'groups' was returned, but 'oidc-group-field' is not set, check your coder oidc settings")
935-
}
936-
937892
// The username is a required property in Coder. We make a best-effort
938893
// attempt at using what the claims provide, but if that fails we will
939894
// generate a random username.
@@ -970,6 +925,21 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
970925
picture, _ = pictureRaw.(string)
971926
}
972927

928+
usingGroups, groups, err := api.oidcGroups(ctx, mergedClaims)
929+
if err != nil {
930+
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
931+
Message: "Failed to sync groups from OIDC claims",
932+
Detail: err.Error(),
933+
})
934+
return
935+
}
936+
937+
roles, ok := api.oidcRoles(ctx, rw, r, mergedClaims)
938+
if !ok {
939+
// oidcRoles writes the error to the response writer for us.
940+
return
941+
}
942+
973943
user, link, err := findLinkedUser(ctx, api.Database, oidcLinkedID(idToken), email)
974944
if err != nil {
975945
logger.Error(ctx, "oauth2: unable to find linked user", slog.F("email", email), slog.Error(err))
@@ -980,63 +950,6 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
980950
return
981951
}
982952

983-
roles := api.OIDCConfig.UserRolesDefault
984-
if api.OIDCConfig.RoleSyncEnabled() {
985-
rolesRow, ok := mergedClaims[api.OIDCConfig.UserRoleField]
986-
if !ok {
987-
// If no claim is provided than we can assume the user is just
988-
// a member. This is because there is no way to tell the difference
989-
// between []string{} and nil for OIDC claims. IDPs omit claims
990-
// if they are empty ([]string{}).
991-
// Use []interface{}{} so the next typecast works.
992-
rolesRow = []interface{}{}
993-
}
994-
995-
rolesInterface, ok := rolesRow.([]interface{})
996-
if !ok {
997-
api.Logger.Error(ctx, "oidc claim user roles field was an unknown type",
998-
slog.F("type", fmt.Sprintf("%T", rolesRow)),
999-
)
1000-
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
1001-
Status: http.StatusInternalServerError,
1002-
HideStatus: true,
1003-
Title: "Login disabled until OIDC config is fixed",
1004-
Description: fmt.Sprintf("Roles claim must be an array of strings, type found: %T. Disabling role sync will allow login to proceed.", rolesRow),
1005-
RetryEnabled: false,
1006-
DashboardURL: "/login",
1007-
})
1008-
return
1009-
}
1010-
1011-
api.Logger.Debug(ctx, "roles returned in oidc claims",
1012-
slog.F("len", len(rolesInterface)),
1013-
slog.F("roles", rolesInterface),
1014-
)
1015-
for _, roleInterface := range rolesInterface {
1016-
role, ok := roleInterface.(string)
1017-
if !ok {
1018-
api.Logger.Error(ctx, "invalid oidc user role type",
1019-
slog.F("type", fmt.Sprintf("%T", rolesRow)),
1020-
)
1021-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
1022-
Message: fmt.Sprintf("Invalid user role type. Expected string, got: %T", roleInterface),
1023-
})
1024-
return
1025-
}
1026-
1027-
if mappedRoles, ok := api.OIDCConfig.UserRoleMapping[role]; ok {
1028-
if len(mappedRoles) == 0 {
1029-
continue
1030-
}
1031-
// Mapped roles are added to the list of roles
1032-
roles = append(roles, mappedRoles...)
1033-
continue
1034-
}
1035-
1036-
roles = append(roles, role)
1037-
}
1038-
}
1039-
1040953
// If a new user is authenticating for the first time
1041954
// the audit action is 'register', not 'login'
1042955
if user.ID == uuid.Nil {
@@ -1053,9 +966,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
1053966
Email: email,
1054967
Username: username,
1055968
AvatarURL: picture,
1056-
UsingGroups: usingGroups,
1057969
UsingRoles: api.OIDCConfig.RoleSyncEnabled(),
1058970
Roles: roles,
971+
UsingGroups: usingGroups,
1059972
Groups: groups,
1060973
CreateMissingGroups: api.OIDCConfig.CreateMissingGroups,
1061974
GroupFilter: api.OIDCConfig.GroupFilter,
@@ -1095,6 +1008,123 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
10951008
http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect)
10961009
}
10971010

1011+
// oidcGroups returns the groups for the user from the OIDC claims.
1012+
func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interface{}) (bool, []string, error) {
1013+
logger := api.Logger.Named(userAuthLoggerName)
1014+
usingGroups := false
1015+
var groups []string
1016+
1017+
// If the GroupField is the empty string, then groups from OIDC are not used.
1018+
// This is so we can support manual group assignment.
1019+
if api.OIDCConfig.GroupField != "" {
1020+
usingGroups = true
1021+
groupsRaw, ok := mergedClaims[api.OIDCConfig.GroupField]
1022+
if ok && api.OIDCConfig.GroupField != "" {
1023+
// Convert the []interface{} we get to a []string.
1024+
groupsInterface, ok := groupsRaw.([]interface{})
1025+
if ok {
1026+
api.Logger.Debug(ctx, "groups returned in oidc claims",
1027+
slog.F("len", len(groupsInterface)),
1028+
slog.F("groups", groupsInterface),
1029+
)
1030+
1031+
for _, groupInterface := range groupsInterface {
1032+
group, ok := groupInterface.(string)
1033+
if !ok {
1034+
return false, nil, xerrors.Errorf("Invalid group type. Expected string, got: %T", groupInterface)
1035+
}
1036+
1037+
if mappedGroup, ok := api.OIDCConfig.GroupMapping[group]; ok {
1038+
group = mappedGroup
1039+
}
1040+
1041+
groups = append(groups, group)
1042+
}
1043+
} else {
1044+
api.Logger.Debug(ctx, "groups field was an unknown type",
1045+
slog.F("type", fmt.Sprintf("%T", groupsRaw)),
1046+
)
1047+
}
1048+
}
1049+
}
1050+
1051+
// This conditional is purely to warn the user they might have misconfigured their OIDC
1052+
// configuration.
1053+
if _, groupClaimExists := mergedClaims["groups"]; !usingGroups && groupClaimExists {
1054+
logger.Debug(ctx, "claim 'groups' was returned, but 'oidc-group-field' is not set, check your coder oidc settings")
1055+
}
1056+
1057+
return usingGroups, groups, nil
1058+
}
1059+
1060+
// oidcRoles returns the roles for the user from the OIDC claims.
1061+
// If the function returns false, then the caller should return early.
1062+
// All writes to the response writer are handled by this function.
1063+
// It would be preferred to just return an error, however this function
1064+
// decorates returned errors with the appropriate HTTP status codes and details
1065+
// that are hard to carry in a standard `error` without more work.
1066+
func (api *API) oidcRoles(ctx context.Context, rw http.ResponseWriter, r *http.Request, mergedClaims map[string]interface{}) ([]string, bool) {
1067+
roles := api.OIDCConfig.UserRolesDefault
1068+
if !api.OIDCConfig.RoleSyncEnabled() {
1069+
return roles, true
1070+
}
1071+
1072+
rolesRow, ok := mergedClaims[api.OIDCConfig.UserRoleField]
1073+
if !ok {
1074+
// If no claim is provided than we can assume the user is just
1075+
// a member. This is because there is no way to tell the difference
1076+
// between []string{} and nil for OIDC claims. IDPs omit claims
1077+
// if they are empty ([]string{}).
1078+
// Use []interface{}{} so the next typecast works.
1079+
rolesRow = []interface{}{}
1080+
}
1081+
1082+
rolesInterface, ok := rolesRow.([]interface{})
1083+
if !ok {
1084+
api.Logger.Error(ctx, "oidc claim user roles field was an unknown type",
1085+
slog.F("type", fmt.Sprintf("%T", rolesRow)),
1086+
)
1087+
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
1088+
Status: http.StatusInternalServerError,
1089+
HideStatus: true,
1090+
Title: "Login disabled until OIDC config is fixed",
1091+
Description: fmt.Sprintf("Roles claim must be an array of strings, type found: %T. Disabling role sync will allow login to proceed.", rolesRow),
1092+
RetryEnabled: false,
1093+
DashboardURL: "/login",
1094+
})
1095+
return nil, false
1096+
}
1097+
1098+
api.Logger.Debug(ctx, "roles returned in oidc claims",
1099+
slog.F("len", len(rolesInterface)),
1100+
slog.F("roles", rolesInterface),
1101+
)
1102+
for _, roleInterface := range rolesInterface {
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+
return nil, false
1112+
}
1113+
1114+
if mappedRoles, ok := api.OIDCConfig.UserRoleMapping[role]; ok {
1115+
if len(mappedRoles) == 0 {
1116+
continue
1117+
}
1118+
// Mapped roles are added to the list of roles
1119+
roles = append(roles, mappedRoles...)
1120+
continue
1121+
}
1122+
1123+
roles = append(roles, role)
1124+
}
1125+
return roles, true
1126+
}
1127+
10981128
// claimFields returns the sorted list of fields in the claims map.
10991129
func claimFields(claims map[string]interface{}) []string {
11001130
fields := []string{}

0 commit comments

Comments
 (0)