vikunja/vikunja
6
43
Fork 73
Code Issues 150 Pull Requests 20 Packages Releases 32 Activity

feat: assign users to teams via OIDC claims #1393

Merged
konrad merged 93 commits from viehlieb/api:950_reworked_assign_teams_via_oidc into main 2024-03-02 08:47:12 +00:00
Conversation 34 Commits 93 Files Changed 26 +37 -42
1 changed files with 37 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit c3a1a5062a - Show all commits

79
pkg/modules/auth/openid/openid.go
View File

@ -60,11 +60,11 @@ type Provider struct {
Oauth2Config *oauth2.Config `json:"-"`
}
type claims struct {
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-02-13 10:23:12 +00:00
Outdated
Review
Copy Link

Didn't you declare this already somewhere else?

Didn't you declare this already somewhere else?
Email string `json:"email"`
Name string `json:"name"`
PreferredUsername string `json:"preferred_username"`
Nickname string `json:"nickname"`
VikunjaGroups interface{} `json:"vikunja_groups"`
Email string `json:"email"`
Name string `json:"name"`
PreferredUsername string `json:"preferred_username"`
Nickname string `json:"nickname"`
VikunjaGroups []map[string]interface{} `json:"vikunja_groups"`
}
func init() {
@ -277,41 +277,37 @@ func SignOutFromTeamsByID(s *xorm.Session, u *user.User, teamIDs []int64) (errs
return errs
}
func getTeamDataFromToken(groups interface{}, provider *Provider) (teamData []models.TeamData, errs []error) {
func getTeamDataFromToken(groups []map[string]interface{}, provider *Provider) (teamData []models.TeamData, errs []error) {
teamData = []models.TeamData{}
errs = []error{}
if groups != nil {
el := groups.([]interface{})
for _, data := range el {
team := data.(map[string]interface{})
var name string
var description string
var oidcID string
if team["name"] != nil {
name = team["name"].(string)
}
if team["description"] != nil {
description = team["description"].(string)
}
if team["oidcID"] != nil {
switch t := team["oidcID"].(type) {
case int64:
oidcID = strconv.FormatInt(team["oidcID"].(int64), 10)
case string:
oidcID = string(team["oidcID"].(string))
case float64:
oidcID = strconv.FormatFloat(team["oidcID"].(float64), 'f', -1, 64)
default:
log.Errorf("No oidcID assigned for %v or type %v not supported", team, t)
}
}
if name == "" || oidcID == "" {
log.Errorf("Claim of your custom scope does not hold name or oidcID for automatic group assignment through oidc provider. Please check %s", provider.Name)
errs = append(errs, &user.ErrOpenIDCustomScopeMalformed{})
continue
}
teamData = append(teamData, models.TeamData{TeamName: name, OidcID: oidcID, Description: description})
for _, team := range groups {
var name string
var description string
var oidcID string
if team["name"] != nil {
name = team["name"].(string)
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-02-13 10:35:31 +00:00
Outdated
Review
Copy Link

Please use `strconv.FormatInt

Please use `strconv.FormatInt
viehlieb commented 2023-02-23 14:25:52 +00:00
Outdated
Review
Copy Link

done

done
}
if team["description"] != nil {
description = team["description"].(string)
}
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-12-01 15:01:28 +00:00
Outdated
Review
Copy Link

Please use xorm's In function because that allows you to pass an int slice directly and avoids the need for the conversion you're doing.

Please use xorm's `In` function because that allows you to pass an int slice directly and avoids the need for the conversion you're doing.
if team["oidcID"] != nil {
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-02-13 10:35:56 +00:00
Outdated
Review
Copy Link

Please use strconv.FormatFloat (not 100% sure if the function is really called that)

Please use `strconv.FormatFloat` (not 100% sure if the function is really called that)
switch t := team["oidcID"].(type) {
case int64:
oidcID = strconv.FormatInt(team["oidcID"].(int64), 10)
case string:
oidcID = string(team["oidcID"].(string))
case float64:
oidcID = strconv.FormatFloat(team["oidcID"].(float64), 'f', -1, 64)
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-02-13 10:37:10 +00:00
Outdated
Review
Copy Link

The return here will mean if one team claim is invalid, it will not try to validate any others. Is that what you intended?

The return here will mean if one team claim is invalid, it will not try to validate any others. Is that what you intended?
viehlieb commented 2023-02-23 14:57:17 +00:00
Outdated
Review
Copy Link

changed behaviour to skipping the invalid team

changed behaviour to skipping the invalid team
default:
log.Errorf("No oidcID assigned for %v or type %v not supported", team, t)
}
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-12-01 15:02:59 +00:00
Outdated
Review
Copy Link

Please use _, exists := team["name"] to check if the field exists (exists is a boolean indicating whether the field exists). Same for the other fields.

Please use `_, exists := team["name"]` to check if the field exists (`exists` is a boolean indicating whether the field exists). Same for the other fields.
}
if name == "" || oidcID == "" {
log.Errorf("Claim of your custom scope does not hold name or oidcID for automatic group assignment through oidc provider. Please check %s", provider.Name)
errs = append(errs, &user.ErrOpenIDCustomScopeMalformed{})
continue
}
teamData = append(teamData, models.TeamData{TeamName: name, OidcID: oidcID, Description: description})
}
return teamData, errs
}
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-04-17 16:24:11 +00:00
Outdated
Review
Copy Link

Please rename this to team.

Please rename this to `team`.
konrad commented 2023-10-18 15:20:28 +00:00
Outdated
Review
Copy Link

What's the advantage of this method over team.Create?

What's the advantage of this method over `team.Create`?
viehlieb commented 2023-11-07 14:16:57 +00:00
Outdated
Review
Copy Link

i think it was cyclomatic complexity.

i think it was cyclomatic complexity.
@ -327,8 +323,8 @@ func CreateTeamWithData(s *xorm.Session, teamData models.TeamData, u *user.User)
}
// this functions creates an array of existing teams that was generated from the oidc data.
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-04-17 16:25:19 +00:00
Outdated
Review
Copy Link

Which oidcID? In the teamData? Isn't that always a string, due to the type system? Then the comment is redundant, please remove or clarify further.

Which oidcID? In the `teamData`? Isn't that always a string, due to the type system? Then the comment is redundant, please remove or clarify further.
func GetOrCreateTeamsByOIDCAndNames(s *xorm.Session, teamData []models.TeamData, u *user.User) (te []models.Team, err error) {
te = []models.Team{}
func GetOrCreateTeamsByOIDCAndNames(s *xorm.Session, teamData []models.TeamData, u *user.User) (te []*models.Team, err error) {
te = []*models.Team{}
// Procedure can only be successful if oidcID is set and converted to string
for _, oidcTeam := range teamData {
team, err := models.GetTeamByOidcIDAndName(s, oidcTeam.OidcID, oidcTeam.TeamName)
@ -338,12 +334,11 @@ func GetOrCreateTeamsByOIDCAndNames(s *xorm.Session, teamData []models.TeamData,
if err != nil {
return te, err
}
te = append(te, *newTeam)
te = append(te, newTeam)
} else {
log.Debugf("Team with oidc_id %v and name %v already exists.", team.OidcID, team.Name)
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-02-13 10:39:05 +00:00
Outdated
Review
Copy Link

Please make this debug message more descriptive or remove it alltogether.

Please make this debug message more descriptive or remove it alltogether.
te = append(te, team)
te = append(te, &team)
}
}
return te, err
}