vikunja/vikunja
7
43
Fork 73
Code Issues 151 Pull Requests 16 Packages Releases 32 Activity

fix: OIDC team IDs should be bound to an issuer

Browse Source 
This commit introduces changes to make sure that (a) the Team name is no longer used when comparing teams for existence and (b) the issuer if stored instead, as the oidcID is only guaranteed to be unique within the same provider
This commit is contained in:
Daniel Herrmann 2024-03-04 17:30:33 +01:00 committed by waza-ari
parent 144f142acc
commit 7adf75487b
6 changed files with 67 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
pkg/db/fixtures/teams.yml
View File

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

43
pkg/migration/20240304153738.go Normal file
View 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
},
})
}

8
pkg/models/error.go
View File

@ -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. [OidcID : %v] [Issuer Name: %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 oidcId and issuer."}
}
// ErrOIDCTeamsDoNotExistForUser represents an error where an oidcTeam does not exist for the user

8
pkg/models/teams.go
View File

@ -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"`
@ -131,14 +133,14 @@ func GetTeamByID(s *xorm.Session, id int64) (team *Team, err error) {
// GetTeamByOidcID returns a team matching the given oidc_id
// For oidc team creation oidcID and Name need to be set
func GetTeamByOidcID(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 = ?", oidcID).
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
}

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

@ -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.GetTeamByOidcID(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
}

8
pkg/modules/auth/openid/openid_test.go
View File

@ -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)