vikunja/vikunja
6
43
Fork 73
Code Issues 150 Pull Requests 21 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 -64
2 changed files with 0 additions and 64 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit af2460c1f9 - Show all commits

29
pkg/models/teams.go
View File

@ -159,35 +159,6 @@ func FindAllOidcTeamIDsForUser(s *xorm.Session, userID int64) (ts []int64, err e
return ts, nil
}
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-02-13 10:20:07 +00:00
Outdated
Review
Copy Link

This error message should contain the name as well.

This error message should contain the name as well.
// GetTeamByOidcIDAndName gets teams where oidc_id and name match parameters
// For oidc team creation oidcID and Name need to be set
func GetTeamByOidcIDAndName(s *xorm.Session, oidcID string, teamName string) (team Team, err error) {
has, err := s.
Table("teams").
Where("oidc_id = ? AND name = ?", oidcID, teamName).
Asc("id").
Limit(1).
Get(&team)
if !has || err != nil {
return team, ErrOIDCTeamDoesNotExist{teamName, oidcID}
}
return team, err
}
func FindAllOidcTeamIDsForUser(s *xorm.Session, userID int64) (ts []int64, err error) {
err = s.
Table("team_members").
Where("user_id = ? ", userID).
Join("RIGHT", "teams", "teams.id = team_members.team_id").
Where("teams.oidc_id != ? AND teams.oidc_id IS NOT NULL", "").
Cols("teams.id").
Find(&ts)
if ts == nil || err != nil {
return ts, ErrOIDCTeamsDoNotExistForUser{userID}
}
return ts, nil
}
func addMoreInfoToTeams(s *xorm.Session, teams []*Team) (err error) {
if len(teams) == 0 {

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

@ -232,34 +232,6 @@ func HandleCallback(c echo.Context) error {
return auth.NewUserAuthTokenResponse(u, c, false)
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-04-17 16:21:36 +00:00
Outdated
Review
Copy Link

Replace with

if len(teamData) < 1 {
	return
}
Replace with ``` if len(teamData) < 1 { return }
viehlieb commented 2023-05-03 15:09:07 +00:00
Outdated
Review
Copy Link

It could well be, that only a single team is received through the oidc token.

It could well be, that only a single team is received through the oidc token.
konrad commented 2023-05-06 16:13:14 +00:00
Outdated
Review
Copy Link

But len(teamData) < 1 will only be true if there are 0 teams received?

I should have phrased it better, this is equivalent:

if len(teamData) == 0 {
	return
}
But `len(teamData) < 1` will only be true if there are 0 teams received? I should have phrased it better, this is equivalent: ``` if len(teamData) == 0 { return } ```
viehlieb commented 2023-05-08 11:43:19 +00:00
Outdated
Review
Copy Link

yes, that's true of course. i do not know why i read it differently.

yes, that's true of course. i do not know why i read it differently.
👍 1
}
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-02-13 10:25:53 +00:00
Outdated
Review
Copy Link

This should be an Error log message.

This should be an `Error` log message.
func AssignOrCreateUserToTeams(s *xorm.Session, u *user.User, teamData []models.OIDCTeamData) (oidcTeams []int64, err error) {
if len(teamData) > 0 {
// check if we have seen these teams before.
// find or create Teams and assign user as teammember.
teams, err := GetOrCreateTeamsByOIDCAndNames(s, teamData, u)
if err != nil {
log.Errorf("Error verifying team for %v, got %v. Error: %v", u.Name, teams, err)
return nil, err
}
//find old teams for user through oidc
oldOidcTeams, err := models.FindAllOidcTeamIDsForUser(s, u.ID)
if err != nil {
log.Errorf("No Oidc Teams found for user %v", err)
}
oidcTeams, err := AssignOrCreateUserToTeams(s, u, teamData)
if err != nil {
log.Errorf("Could not proceed with group routine %v", err)
}
errs = RemoveUserFromTeamsByIds(s, u, utils.NotIn(oldOidcTeams, oidcTeams))
for _, err := range errs {
log.Errorf("Found Error while signing out from teams %v", err)
}
oidcTeams = append(oidcTeams, team.ID)
}
return oidcTeams, err
<<<<<<< HEAD
func AssignOrCreateUserToTeams(s *xorm.Session, u *user.User, teamData []models.OIDCTeamData) (oidcTeams []int64, err error) {
if len(teamData) == 0 {
return
@ -284,14 +256,7 @@ func AssignOrCreateUserToTeams(s *xorm.Session, u *user.User, teamData []models.
}
return oidcTeams, err
}
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-10-18 15:19:17 +00:00
Outdated
Review
Copy Link

why not do this in one sql query?

why not do this in one sql query?
viehlieb commented 2023-11-07 15:14:54 +00:00
Outdated
Review
Copy Link

You mean something like:

func RemoveUserFromTeamsByIds(s *xorm.Session, u *user.User, teamIDs []int64) (err error) {

	if len(teamIDs) < 1 {
		return nil
	}

	strSlice := make([]string, len(teamIDs))
	for i, num := range teamIDs {
		strSlice[i] = strconv.FormatInt(num, 10)
	}

	sql := fmt.Sprintf(`DELETE FROM team_members WHERE user_id = %d AND team_id IN (%v)`, u.ID, strings.Join(strSlice, ","))

	_, err = s.Exec(sql)

	return err
You mean something like: ``` func RemoveUserFromTeamsByIds(s *xorm.Session, u *user.User, teamIDs []int64) (err error) { if len(teamIDs) < 1 { return nil } strSlice := make([]string, len(teamIDs)) for i, num := range teamIDs { strSlice[i] = strconv.FormatInt(num, 10) } sql := fmt.Sprintf(`DELETE FROM team_members WHERE user_id = %d AND team_id IN (%v)`, u.ID, strings.Join(strSlice, ",")) _, err = s.Exec(sql) return err ```
konrad commented 2023-11-07 16:30:09 +00:00
Outdated
Review
Copy Link

Yes, but with xorm:

	_, err = s.Where(builder.And(builder.Where("user_id = ?", u.ID), builder.In("team_id", teamIDs)).Delete(&TeamMember{})
	return err
Yes, but with xorm: ``` _, err = s.Where(builder.And(builder.Where("user_id = ?", u.ID), builder.In("team_id", teamIDs)).Delete(&TeamMember{}) return err ```
viehlieb commented 2023-11-08 11:51:31 +00:00
Outdated
Review
Copy Link

went for:

_, err = s.Where("team_id IN (?) AND user_id = ?", strings.Join(strSlice, ","), u.ID).Delete(&models.TeamMember{})

went for: `_, err = s.Where("team_id IN (?) AND user_id = ?", strings.Join(strSlice, ","), u.ID).Delete(&models.TeamMember{})`
konrad commented 2023-11-09 11:44:09 +00:00
Outdated
Review
Copy Link

That's still working agains the orm - makes things more complicated in the end.

Please use xorm's In function instead.

Something like this might work as well:

_, err = s.And("user_id = ?", u.ID).In("team_id", teamIDs).Delete(&TeamMember{})
	return err
That's still working agains the orm - makes things more complicated in the end. Please use xorm's `In` function instead. Something like this might work as well: ```go _, err = s.And("user_id = ?", u.ID).In("team_id", teamIDs).Delete(&TeamMember{}) return err ```
<<<<<<< HEAD
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-02-13 10:29:13 +00:00
Outdated
Review
Copy Link

Please return the error instead of logging only.

Please return the error instead of logging only.
konrad commented 2023-02-14 11:38:32 +00:00
Outdated
Review
Copy Link

Please don't ignore the errors.

Please don't ignore the errors.
viehlieb commented 2023-02-23 14:43:36 +00:00
Outdated
Review
Copy Link

not ignored now

not ignored now
=======
>>>>>>> 169b668c... remove left over function GetMemberCount, rename function SignOut to RemoveFrom
=======
}
>>>>>>> 3fdbd53b... work on openid to just start group workflow when teamData is available
func RemoveUserFromTeamsByIds(s *xorm.Session, u *user.User, teamIDs []int64) (errs []error) {
errs = []error{}
for _, teamID := range teamIDs {