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

9
pkg/models/team_members.go
View File

@ -115,15 +115,6 @@ func (tm *TeamMember) CheckMembership(s *xorm.Session) (exists bool, err error)
return exists, err
}
func (tm *TeamMember) GetMemberCount(s *xorm.Session) (memberCount int64, err error) {
member := TeamMember{}
memberCount, err = s.
Where("team_id = ?", tm.TeamID).
Cols("user_id").
Count(&member)
return memberCount, 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.
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-02-23 15:24:19 +00:00
Outdated
Review
Copy Link

Couldn't you just return the result here directly?

Couldn't you just return the result here directly?
viehlieb commented 2023-03-10 12:10:00 +00:00
Outdated
Review
Copy Link

In fact this function is not needed anymore for this feature, so I'll just remove it.
It was used for finding out whether a user should be signed out from team or deleted.

In fact this function is not needed anymore for this feature, so I'll just remove it. It was used for finding out whether a user should be signed out from team or deleted.
👍 1

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

@ -216,7 +216,7 @@ func HandleCallback(c echo.Context) error {
if err != nil {
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-02-13 10:24:55 +00:00
Outdated
Review
Copy Link

Please don't ignore the error.

Please don't ignore the error.
log.Errorf("Could not proceed with group routine %v", err)
}
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-10-18 15:17:33 +00:00
Outdated
Review
Copy Link

Please don't call this "Sign out". That's a different thing.

Please don't call this "Sign out". That's a different thing.
errs = SignOutFromTeamsByID(s, u, utils.NotIn(oldOidcTeams, oidcTeams))
errs = RemoveUserFromTeamsByIds(s, u, utils.NotIn(oldOidcTeams, oidcTeams))
log.Errorf("%v", errs)
for _, err := range errs {
log.Errorf("Found Error while signing out from teams %v", err)
@ -255,7 +255,7 @@ func AssignOrCreateUserToTeams(s *xorm.Session, u *user.User, teamData []models.
return oidcTeams, err
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-02-23 15:31:02 +00:00
Outdated
Review
Copy Link

Please call this something like RemoveUserFromTeamsByID so that it is clear how destructive this is.

Please call this something like `RemoveUserFromTeamsByID` so that it is clear how destructive this is.
}
func SignOutFromTeamsByID(s *xorm.Session, u *user.User, teamIDs []int64) (errs []error) {
func RemoveUserFromTeamsByIds(s *xorm.Session, u *user.User, teamIDs []int64) (errs []error) {
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 ```
errs = []error{}
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
for _, teamID := range teamIDs {
tm := models.TeamMember{TeamID: teamID, UserID: u.ID, Username: u.Username}