Revert "refactor: move logic to create a team outside of Create and add additional parameter to control default admin behaviour"

This reverts commit 3bad0afbc3.

go.mod

@@ -71,7 +71,7 @@ require (
 	golang.org/x/oauth2 v0.17.0
 	golang.org/x/sync v0.6.0
 	golang.org/x/sys v0.18.0
-	golang.org/x/term v0.17.0
+	golang.org/x/term v0.18.0
 	golang.org/x/text v0.14.0
 	gopkg.in/d4l3k/messagediff.v1 v1.2.1
 	gopkg.in/yaml.v3 v3.0.1

go.sum

@@ -741,6 +741,8 @@ golang.org/x/term v0.16.0 h1:m+B6fahuftsE9qjo0VWp2FW0mB3MTJvR0BaMQrq0pmE=
 golang.org/x/term v0.16.0/go.mod h1:yn7UURbUtPyrVJPGPq404EukNFxcm/foM+bV/bfcDsY=
 golang.org/x/term v0.17.0 h1:mkTF7LCd6WGJNL3K1Ad7kwxNfYAW6a8a8QqtMblp/4U=
 golang.org/x/term v0.17.0/go.mod h1:lLRBjIVuehSbZlaOtGMbcMncT+aqLLLmKrsjNrUguwk=
+golang.org/x/term v0.18.0 h1:FcHjZXDMxI8mM3nwhX9HlKop4C0YQvCVCdwYl2wOtE8=
+golang.org/x/term v0.18.0/go.mod h1:ILwASektA3OnRv7amZ1xhE/KTR+u50pbXfZ03+6Nx58=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk=
 golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=

pkg/db/fixtures/teams.yml

@@ -32,4 +32,5 @@
 - id: 14
   name: testteam14
   created_by_id: 7
-  oidc_id: 14
+  oidc_id: 14
+  issuer: "https://some.issuer"

pkg/migration/20240304153738.go

@@ -0,0 +1,43 @@
+// Vikunja is a to-do list application to facilitate your life.
+// Copyright 2018-present Vikunja and contributors. All rights reserved.
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public Licensee as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public Licensee for more details.
+//
+// You should have received a copy of the GNU Affero General Public Licensee
+// along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+package migration
+
+import (
+	"src.techknowlogick.com/xormigrate"
+	"xorm.io/xorm"
+)
+
+type teams20240304153738 struct {
+	Issuer string `xorm:"text null" json:"-"`
+}
+
+func (teams20240304153738) TableName() string {
+	return "teams"
+}
+
+func init() {
+	migrations = append(migrations, &xormigrate.Migration{
+		ID:          "20240304153738",
+		Description: "Add issuer column to teams to track the OIDC provider this issue has bene created by",
+		Migrate: func(tx *xorm.Engine) error {
+			return tx.Sync2(teams20240304153738{})
+		},
+		Rollback: func(tx *xorm.Engine) error {
+			return nil
+		},
+	})
+}

pkg/models/error.go

@@ -1177,10 +1177,10 @@ func (err ErrTeamDoesNotHaveAccessToProject) HTTPError() web.HTTPError {
 	return web.HTTPError{HTTPCode: http.StatusForbidden, Code: ErrCodeTeamDoesNotHaveAccessToProject, Message: "This team does not have access to the project."}
 }
 
-// ErrOIDCTeamDoesNotExist represents an error where a team with specified name and specified oidcId property does not exist
+// ErrOIDCTeamDoesNotExist represents an error where a team with specified oidcId property does not exist for a given issuer
 type ErrOIDCTeamDoesNotExist struct {
 	OidcID string
-	Name   string
+	Issuer string
 }
 
 // IsErrOIDCTeamDoesNotExist checks if an error is ErrOIDCTeamDoesNotExist.
@@ -1191,7 +1191,7 @@ func IsErrOIDCTeamDoesNotExist(err error) bool {
 
 // ErrTeamDoesNotExist represents an error where a team does not exist
 func (err ErrOIDCTeamDoesNotExist) Error() string {
-	return fmt.Sprintf("No team with that name and valid oidcId could be found. [Team Name: %v] [OidcID : %v] ", err.Name, err.OidcID)
+	return fmt.Sprintf("No team could be found for the given oidcId and issuer. [OIDC ID : %v] [Issuer: %v] ", err.OidcID, err.Issuer)
 }
 
 // ErrCodeTeamDoesNotExist holds the unique world-error code of this error
@@ -1199,7 +1199,7 @@ const ErrCodeOIDCTeamDoesNotExist = 6008
 
 // HTTPError holds the http error description
 func (err ErrOIDCTeamDoesNotExist) HTTPError() web.HTTPError {
-	return web.HTTPError{HTTPCode: http.StatusNotFound, Code: ErrCodeTeamDoesNotExist, Message: "No team with that name and valid oidcId could be found."}
+	return web.HTTPError{HTTPCode: http.StatusNotFound, Code: ErrCodeTeamDoesNotExist, Message: "No team could be found for the given OIDC ID and issuer."}
 }
 
 // ErrOIDCTeamsDoNotExistForUser represents an error where an oidcTeam does not exist for the user

pkg/models/teams.go

@@ -40,6 +40,8 @@ type Team struct {
 	CreatedByID int64  `xorm:"bigint not null INDEX" json:"-"`
 	// The team's oidc id delivered by the oidc provider
 	OidcID string `xorm:"varchar(250) null" maxLength:"250" json:"oidc_id"`
+	// Contains the issuer extracted from the vikunja_groups claim if this team was created through oidc
+	Issuer string `xorm:"text null" json:"-"`
 
 	// The user who created this team.
 	CreatedBy *user.User `xorm:"-" json:"created_by"`
@@ -129,16 +131,16 @@ func GetTeamByID(s *xorm.Session, id int64) (team *Team, err error) {
 	return
 }
 
-// GetTeamByOidcIDAndName gets teams where oidc_id and name match parameters
+// GetTeamByOidcID returns a team matching the given oidc_id
 // For oidc team creation oidcID and Name need to be set
-func GetTeamByOidcIDAndName(s *xorm.Session, oidcID string, teamName string) (*Team, error) {
+func GetTeamByOidcIDAndIssuer(s *xorm.Session, oidcID string, issuer string) (*Team, error) {
 	team := &Team{}
 	has, err := s.
 		Table("teams").
-		Where("oidc_id = ? AND name = ?", oidcID, teamName).
+		Where("oidc_id = ? AND issuer = ?", oidcID, issuer).
 		Get(team)
 	if !has || err != nil {
-		return nil, ErrOIDCTeamDoesNotExist{teamName, oidcID}
+		return nil, ErrOIDCTeamDoesNotExist{issuer, oidcID}
 	}
 	return team, nil
 }

pkg/modules/auth/openid/openid.go

@@ -215,7 +215,7 @@ func HandleCallback(c echo.Context) error {
 		if err != nil {
 			log.Debugf("No oidc teams found for user %v", err)
 		}
-		oidcTeams, err := AssignOrCreateUserToTeams(s, u, teamData)
+		oidcTeams, err := AssignOrCreateUserToTeams(s, u, teamData, idToken.Issuer)
 		if err != nil {
 			log.Errorf("Could not proceed with group routine %v", err)
 		}
@@ -241,13 +241,13 @@ func HandleCallback(c echo.Context) error {
 	return auth.NewUserAuthTokenResponse(u, c, false)
 }
 
-func AssignOrCreateUserToTeams(s *xorm.Session, u *user.User, teamData []*models.OIDCTeam) (oidcTeams []int64, err error) {
+func AssignOrCreateUserToTeams(s *xorm.Session, u *user.User, teamData []*models.OIDCTeam, issuer string) (oidcTeams []int64, err error) {
 	if len(teamData) == 0 {
 		return
 	}
 	// check if we have seen these teams before.
 	// find or create Teams and assign user as teammember.
-	teams, err := GetOrCreateTeamsByOIDCAndNames(s, teamData, u)
+	teams, err := GetOrCreateTeamsByOIDC(s, teamData, u, issuer)
 	if err != nil {
 		log.Errorf("Error verifying team for %v, got %v. Error: %v", u.Name, teams, err)
 		return nil, err
@@ -333,28 +333,29 @@ func getOIDCTeamName(name string) string {
 	return name + " (OIDC)"
 }
 
-func CreateOIDCTeam(s *xorm.Session, teamData *models.OIDCTeam, u *user.User) (team *models.Team, err error) {
+func CreateOIDCTeam(s *xorm.Session, teamData *models.OIDCTeam, u *user.User, issuer string) (team *models.Team, err error) {
 	team = &models.Team{
 		Name:        getOIDCTeamName(teamData.Name),
 		Description: teamData.Description,
 		OidcID:      teamData.OidcID,
+		Issuer:      issuer,
 	}
 	err = team.Create(s, u)
 	return team, err
 }
 
-// GetOrCreateTeamsByOIDCAndNames returns a slice of teams which were generated from the oidc data. If a team did not exist previously it is automatically created.
-func GetOrCreateTeamsByOIDCAndNames(s *xorm.Session, teamData []*models.OIDCTeam, u *user.User) (te []*models.Team, err error) {
+// GetOrCreateTeamsByOIDC returns a slice of teams which were generated from the oidc data. If a team did not exist previously it is automatically created.
+func GetOrCreateTeamsByOIDC(s *xorm.Session, teamData []*models.OIDCTeam, u *user.User, issuer string) (te []*models.Team, err error) {
 	te = []*models.Team{}
 	// Procedure can only be successful if oidcID is set
 	for _, oidcTeam := range teamData {
-		team, err := models.GetTeamByOidcIDAndName(s, oidcTeam.OidcID, oidcTeam.Name)
+		team, err := models.GetTeamByOidcIDAndIssuer(s, oidcTeam.OidcID, issuer)
 		if err != nil && !models.IsErrOIDCTeamDoesNotExist(err) {
 			return nil, err
 		}
 		if err != nil && models.IsErrOIDCTeamDoesNotExist(err) {
 			log.Debugf("Team with oidc_id %v and name %v does not exist. Creating team… ", oidcTeam.OidcID, oidcTeam.Name)
-			newTeam, err := CreateOIDCTeam(s, oidcTeam, u)
+			newTeam, err := CreateOIDCTeam(s, oidcTeam, u, issuer)
 			if err != nil {
 				return te, err
 			}

pkg/modules/auth/openid/openid_test.go

@@ -118,7 +118,7 @@ func TestGetOrCreateUser(t *testing.T) {
 			require.NoError(t, err)
 		}
 		require.NoError(t, err)
-		oidcTeams, err := AssignOrCreateUserToTeams(s, u, teamData)
+		oidcTeams, err := AssignOrCreateUserToTeams(s, u, teamData, "https://some.issuer")
 		require.NoError(t, err)
 		err = s.Commit()
 		require.NoError(t, err)
@@ -152,7 +152,7 @@ func TestGetOrCreateUser(t *testing.T) {
 		for _, err := range errs {
 			require.NoError(t, err)
 		}
-		oidcTeams, err := AssignOrCreateUserToTeams(s, u, teamData)
+		oidcTeams, err := AssignOrCreateUserToTeams(s, u, teamData, "https://some.issuer")
 		require.NoError(t, err)
 		err = s.Commit()
 		require.NoError(t, err)
@@ -181,7 +181,7 @@ func TestGetOrCreateUser(t *testing.T) {
 		}
 		oldOidcTeams, err := models.FindAllOidcTeamIDsForUser(s, u.ID)
 		require.NoError(t, err)
-		oidcTeams, err := AssignOrCreateUserToTeams(s, u, teamData)
+		oidcTeams, err := AssignOrCreateUserToTeams(s, u, teamData, "https://some.issuer")
 		require.NoError(t, err)
 		teamIDsToLeave := utils.NotIn(oldOidcTeams, oidcTeams)
 		require.NoError(t, err)
@@ -222,7 +222,7 @@ func TestGetOrCreateUser(t *testing.T) {
 		}
 		oldOidcTeams, err := models.FindAllOidcTeamIDsForUser(s, u.ID)
 		require.NoError(t, err)
-		oidcTeams, err := AssignOrCreateUserToTeams(s, u, teamData)
+		oidcTeams, err := AssignOrCreateUserToTeams(s, u, teamData, "https://some.issuer")
 		require.NoError(t, err)
 		teamIDsToLeave := utils.NotIn(oldOidcTeams, oidcTeams)
 		require.NoError(t, err)