diff --git a/pkg/models/team_members.go b/pkg/models/team_members.go index ced46aeed..8af8aa59b 100644 --- a/pkg/models/team_members.go +++ b/pkg/models/team_members.go @@ -44,7 +44,6 @@ func (tm *TeamMember) Create(s *xorm.Session, a web.Auth) (err error) { if err != nil { return err } - // Check if the user exists member, err := user2.GetUserByUsername(s, tm.Username) if err != nil { @@ -113,12 +112,18 @@ func (tm *TeamMember) CheckMembership(s *xorm.Session) (exists bool, err error) exists, err = s. Where("team_id = ? AND user_id = ?", tm.TeamID, tm.UserID). Get(&TeamMember{}) - if exists && err == nil { - return - } return exists, err } +func (tm *TeamMember) GetMemberCount(s *xorm.Session) (memberCount int, err error) { + members := []TeamMember{} + err = s. + Where("team_id = ?", tm.TeamID). + Cols("user_id"). + Find(&members) + return len(members), err +} + // Update toggles a team member's admin status // @Summary Toggle a team member's admin status // @Description If a user is team admin, this will make them member and vise-versa. diff --git a/pkg/modules/auth/openid/openid.go b/pkg/modules/auth/openid/openid.go index bbe0ee12c..b4f4e6904 100644 --- a/pkg/modules/auth/openid/openid.go +++ b/pkg/modules/auth/openid/openid.go @@ -20,24 +20,24 @@ import ( "context" "encoding/json" "errors" - "fmt" "math/rand" "net/http" + "strconv" "time" "code.vikunja.io/web/handler" "code.vikunja.io/api/pkg/db" - "xorm.io/xorm" - "code.vikunja.io/api/pkg/log" "code.vikunja.io/api/pkg/models" "code.vikunja.io/api/pkg/modules/auth" "code.vikunja.io/api/pkg/user" + "code.vikunja.io/api/pkg/utils" "github.com/coreos/go-oidc/v3/oidc" petname "github.com/dustinkirkland/golang-petname" "github.com/labstack/echo/v4" "golang.org/x/oauth2" + "xorm.io/xorm" ) // Callback contains the callback after an auth request was made and redirected @@ -64,7 +64,6 @@ type claims struct { Name string `json:"name"` PreferredUsername string `json:"preferred_username"` Nickname string `json:"nickname"` - Teams []string `json:"groups"` VikunjaGroups interface{} `json:"vikunja_groups"` } @@ -193,7 +192,6 @@ func HandleCallback(c echo.Context) error { // Check if we have seen this user before u, err := getOrCreateUser(s, cl, idToken.Issuer, idToken.Subject) - if err != nil { _ = s.Rollback() log.Errorf("Error creating new user for provider %s: %v", provider.Name, err) @@ -201,14 +199,18 @@ func HandleCallback(c echo.Context) error { } // does the oidc token contain well formed "vikunja_groups" through vikunja_scope - teamData, err := getTeamDataFromToken(cl.VikunjaGroups, provider) - if err != nil { - log.Errorf("Error creating teams for user and vikunja groups %s: %v", cl.VikunjaGroups, err) - return handler.HandleHTTPError(err, c) + teamData, errs := getTeamDataFromToken(cl.VikunjaGroups, provider) + if len(errs) > 0 { + for _, err := range errs { + log.Errorf("Error creating teams for user and vikunja groups %s: %v", cl.VikunjaGroups, err) + } } //find old teams for user through oidc - oldOidcTeams, _ := models.FindAllOidcTeamIDsForUser(s, u.ID) + oldOidcTeams, err := models.FindAllOidcTeamIDsForUser(s, u.ID) + if err != nil { + log.Errorf("No Oidc Teams found for user", err) + } var oidcTeams []int64 if len(teamData) > 0 { // check if we have seen these teams before. @@ -220,20 +222,21 @@ func HandleCallback(c echo.Context) error { return err } for _, team := range teams { - tm := models.TeamMember{TeamID: team.ID, Username: u.Username} + tm := models.TeamMember{TeamID: team.ID, UserID: u.ID, Username: u.Username} exists, err := tm.CheckMembership(s) if !exists { err = tm.Create(s, u) if err != nil { - log.Debugf("Could not assign %v to %v. %v", u.Username, team.Name, err) + log.Errorf("Could not assign %v to %v. %v", u.Username, team.Name, err) } - } else { - log.Debugf("Team exists? %v or error: %v", exists, err) } oidcTeams = append(oidcTeams, team.ID) } } - SignOutFromOrDeleteTeamsByID(s, u, notIn(oldOidcTeams, oidcTeams)) + errs = SignOutFromOrDeleteTeamsByID(s, u, utils.NotIn(oldOidcTeams, oidcTeams)) + for _, err := range errs { + log.Errorf("Found Error while signing out from teams %v", err) + } err = s.Commit() if err != nil { _ = s.Rollback() @@ -244,26 +247,35 @@ func HandleCallback(c echo.Context) error { return auth.NewUserAuthTokenResponse(u, c, false) } -func SignOutFromOrDeleteTeamsByID(s *xorm.Session, u *user.User, teamIDs []int64) { +func SignOutFromOrDeleteTeamsByID(s *xorm.Session, u *user.User, teamIDs []int64) (errs []error) { + errs = []error{} for _, teamID := range teamIDs { - tm := models.TeamMember{TeamID: teamID, Username: u.Username} - err := tm.Delete(s, u) - if err != nil { + tm := models.TeamMember{TeamID: teamID, UserID: u.ID, Username: u.Username} + exists, err := tm.CheckMembership(s) + memberCount, _ := tm.GetMemberCount(s) + if !exists { + continue + } + err = tm.Delete(s, u) + // if you cannot delete the team_member + if err != nil || memberCount <= 1 { team, err := models.GetTeamByID(s, teamID) if err != nil { - log.Errorf("Cannot find team with id: %v, err: %v", teamID, err) - } else { - err = team.Delete(s, u) - if err != nil { - log.Errorf("Cannot delete team %v", err) - } + errs = append(errs, err) + continue + } + err = team.Delete(s, u) + if err != nil { + errs = append(errs, err) } } } + return errs } -func getTeamDataFromToken(groups interface{}, provider *Provider) (teamData []models.TeamData, err error) { +func getTeamDataFromToken(groups interface{}, provider *Provider) (teamData []models.TeamData, errs []error) { teamData = []models.TeamData{} + errs = []error{} if groups != nil { el := groups.([]interface{}) for _, data := range el { @@ -280,24 +292,24 @@ func getTeamDataFromToken(groups interface{}, provider *Provider) (teamData []mo if team["oidcID"] != nil { switch t := team["oidcID"].(type) { case int64: - oidcID = fmt.Sprintf("%v", team["oidcID"]) + oidcID = strconv.FormatInt(team["oidcID"].(int64), 10) case string: oidcID = string(team["oidcID"].(string)) case float64: - fl := float64(team["oidcID"].(float64)) - oidcID = fmt.Sprintf("%v", fl) + 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) - return teamData, &user.ErrOpenIDCustomScopeMalformed{} + errs = append(errs, &user.ErrOpenIDCustomScopeMalformed{}) + continue } teamData = append(teamData, models.TeamData{TeamName: name, OidcID: oidcID, Description: description}) } } - return teamData, nil + return teamData, errs } func CreateTeamWithData(s *xorm.Session, teamData models.TeamData, u *user.User) (team *models.Team, err error) { @@ -306,8 +318,6 @@ func CreateTeamWithData(s *xorm.Session, teamData models.TeamData, u *user.User) Description: teamData.Description, OidcID: teamData.OidcID, } - log.Debugf("Teams: %v", tea.OidcID) - err = tea.Create(s, u) return tea, err } @@ -331,7 +341,6 @@ func GetOrCreateTeamsByOIDCAndNames(s *xorm.Session, teamData []models.TeamData, } } - log.Debugf("Array: %v", te) return te, err } @@ -407,23 +416,3 @@ func getOrCreateUser(s *xorm.Session, cl *claims, issuer, subject string) (u *us return } - -// find the elements which appear in slice1,but not in slice2 -func notIn(slice1 []int64, slice2 []int64) []int64 { - var diff []int64 - - for _, s1 := range slice1 { - found := false - for _, s2 := range slice2 { - if s1 == s2 { - found = true - break - } - } - // String not found. We add it to return slice - if !found { - diff = append(diff, s1) - } - } - return diff -}