fix(auth): use (issuer, name) to check for uniqueness of oidc teams (#2152)
All checks were successful
continuous-integration/drone/push Build is passing
All checks were successful
continuous-integration/drone/push Build is passing
The change introduced in #2150 introduces a bug where a Team would be re-created every time a user logs in, as the check if a team already exists was based on both the unique `oidcID` and the `name`. This PR proposes to only base the check on the ID, as this should be unique. Co-authored-by: Daniel Herrmann <daniel.herrmann1@gmail.com> Reviewed-on: #2152 Reviewed-by: konrad <k@knt.li> Co-authored-by: waza-ari <daniel.herrmann@makerspace-darmstadt.de> Co-committed-by: waza-ari <daniel.herrmann@makerspace-darmstadt.de>
This commit is contained in:
parent
2414b580c1
commit
a3154e805c
|
@ -33,3 +33,4 @@
|
||||||
name: testteam14
|
name: testteam14
|
||||||
created_by_id: 7
|
created_by_id: 7
|
||||||
oidc_id: 14
|
oidc_id: 14
|
||||||
|
issuer: "https://some.issuer"
|
43
pkg/migration/20240304153738.go
Normal file
43
pkg/migration/20240304153738.go
Normal file
|
@ -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
|
||||||
|
},
|
||||||
|
})
|
||||||
|
}
|
|
@ -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."}
|
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 {
|
type ErrOIDCTeamDoesNotExist struct {
|
||||||
OidcID string
|
OidcID string
|
||||||
Name string
|
Issuer string
|
||||||
}
|
}
|
||||||
|
|
||||||
// IsErrOIDCTeamDoesNotExist checks if an error is ErrOIDCTeamDoesNotExist.
|
// 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
|
// ErrTeamDoesNotExist represents an error where a team does not exist
|
||||||
func (err ErrOIDCTeamDoesNotExist) Error() string {
|
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
|
// ErrCodeTeamDoesNotExist holds the unique world-error code of this error
|
||||||
|
@ -1199,7 +1199,7 @@ const ErrCodeOIDCTeamDoesNotExist = 6008
|
||||||
|
|
||||||
// HTTPError holds the http error description
|
// HTTPError holds the http error description
|
||||||
func (err ErrOIDCTeamDoesNotExist) HTTPError() web.HTTPError {
|
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
|
// ErrOIDCTeamsDoNotExistForUser represents an error where an oidcTeam does not exist for the user
|
||||||
|
|
|
@ -40,6 +40,8 @@ type Team struct {
|
||||||
CreatedByID int64 `xorm:"bigint not null INDEX" json:"-"`
|
CreatedByID int64 `xorm:"bigint not null INDEX" json:"-"`
|
||||||
// The team's oidc id delivered by the oidc provider
|
// The team's oidc id delivered by the oidc provider
|
||||||
OidcID string `xorm:"varchar(250) null" maxLength:"250" json:"oidc_id"`
|
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.
|
// The user who created this team.
|
||||||
CreatedBy *user.User `xorm:"-" json:"created_by"`
|
CreatedBy *user.User `xorm:"-" json:"created_by"`
|
||||||
|
@ -129,16 +131,16 @@ func GetTeamByID(s *xorm.Session, id int64) (team *Team, err error) {
|
||||||
return
|
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
|
// 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{}
|
team := &Team{}
|
||||||
has, err := s.
|
has, err := s.
|
||||||
Table("teams").
|
Table("teams").
|
||||||
Where("oidc_id = ? AND name = ?", oidcID, teamName).
|
Where("oidc_id = ? AND issuer = ?", oidcID, issuer).
|
||||||
Get(team)
|
Get(team)
|
||||||
if !has || err != nil {
|
if !has || err != nil {
|
||||||
return nil, ErrOIDCTeamDoesNotExist{teamName, oidcID}
|
return nil, ErrOIDCTeamDoesNotExist{issuer, oidcID}
|
||||||
}
|
}
|
||||||
return team, nil
|
return team, nil
|
||||||
}
|
}
|
||||||
|
|
|
@ -215,7 +215,7 @@ func HandleCallback(c echo.Context) error {
|
||||||
if err != nil {
|
if err != nil {
|
||||||
log.Debugf("No oidc teams found for user %v", err)
|
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 {
|
if err != nil {
|
||||||
log.Errorf("Could not proceed with group routine %v", err)
|
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)
|
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 {
|
if len(teamData) == 0 {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
// check if we have seen these teams before.
|
// check if we have seen these teams before.
|
||||||
// find or create Teams and assign user as teammember.
|
// 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 {
|
if err != nil {
|
||||||
log.Errorf("Error verifying team for %v, got %v. Error: %v", u.Name, teams, err)
|
log.Errorf("Error verifying team for %v, got %v. Error: %v", u.Name, teams, err)
|
||||||
return nil, err
|
return nil, err
|
||||||
|
@ -333,28 +333,29 @@ func getOIDCTeamName(name string) string {
|
||||||
return name + " (OIDC)"
|
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{
|
team = &models.Team{
|
||||||
Name: getOIDCTeamName(teamData.Name),
|
Name: getOIDCTeamName(teamData.Name),
|
||||||
Description: teamData.Description,
|
Description: teamData.Description,
|
||||||
OidcID: teamData.OidcID,
|
OidcID: teamData.OidcID,
|
||||||
|
Issuer: issuer,
|
||||||
}
|
}
|
||||||
err = team.Create(s, u)
|
err = team.Create(s, u)
|
||||||
return team, err
|
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.
|
// 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 GetOrCreateTeamsByOIDCAndNames(s *xorm.Session, teamData []*models.OIDCTeam, u *user.User) (te []*models.Team, err error) {
|
func GetOrCreateTeamsByOIDC(s *xorm.Session, teamData []*models.OIDCTeam, u *user.User, issuer string) (te []*models.Team, err error) {
|
||||||
te = []*models.Team{}
|
te = []*models.Team{}
|
||||||
// Procedure can only be successful if oidcID is set
|
// Procedure can only be successful if oidcID is set
|
||||||
for _, oidcTeam := range teamData {
|
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) {
|
if err != nil && !models.IsErrOIDCTeamDoesNotExist(err) {
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
if err != nil && models.IsErrOIDCTeamDoesNotExist(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)
|
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 {
|
if err != nil {
|
||||||
return te, err
|
return te, err
|
||||||
}
|
}
|
||||||
|
|
|
@ -118,7 +118,7 @@ func TestGetOrCreateUser(t *testing.T) {
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
}
|
}
|
||||||
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)
|
require.NoError(t, err)
|
||||||
err = s.Commit()
|
err = s.Commit()
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
|
@ -152,7 +152,7 @@ func TestGetOrCreateUser(t *testing.T) {
|
||||||
for _, err := range errs {
|
for _, err := range errs {
|
||||||
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)
|
require.NoError(t, err)
|
||||||
err = s.Commit()
|
err = s.Commit()
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
|
@ -181,7 +181,7 @@ func TestGetOrCreateUser(t *testing.T) {
|
||||||
}
|
}
|
||||||
oldOidcTeams, err := models.FindAllOidcTeamIDsForUser(s, u.ID)
|
oldOidcTeams, err := models.FindAllOidcTeamIDsForUser(s, u.ID)
|
||||||
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)
|
require.NoError(t, err)
|
||||||
teamIDsToLeave := utils.NotIn(oldOidcTeams, oidcTeams)
|
teamIDsToLeave := utils.NotIn(oldOidcTeams, oidcTeams)
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
|
@ -222,7 +222,7 @@ func TestGetOrCreateUser(t *testing.T) {
|
||||||
}
|
}
|
||||||
oldOidcTeams, err := models.FindAllOidcTeamIDsForUser(s, u.ID)
|
oldOidcTeams, err := models.FindAllOidcTeamIDsForUser(s, u.ID)
|
||||||
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)
|
require.NoError(t, err)
|
||||||
teamIDsToLeave := utils.NotIn(oldOidcTeams, oidcTeams)
|
teamIDsToLeave := utils.NotIn(oldOidcTeams, oidcTeams)
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
|
|
Loading…
Reference in New Issue
Block a user