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

10
pkg/models/team_members.go
View File

@ -115,13 +115,13 @@ func (tm *TeamMember) CheckMembership(s *xorm.Session) (exists bool, err error)
return exists, err
}
func (tm *TeamMember) GetMemberCount(s *xorm.Session) (memberCount int, err error) {
members := []TeamMember{}
err = s.
func (tm *TeamMember) GetMemberCount(s *xorm.Session) (memberCount int64, err error) {
member := TeamMember{}
memberCount, err = s.
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
Where("team_id = ?", tm.TeamID).
Cols("user_id").
Find(&members)
return len(members), err
Count(&member)
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-02-14 11:38:07 +00:00
Outdated
Review
Copy Link

If it's only counting, please use Count().

If it's only counting, please use `Count()`.
viehlieb commented 2023-02-23 14:43:46 +00:00
Outdated
Review
Copy Link

done

done
return memberCount, err
}
// Update toggles a team member's admin status

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

@ -216,7 +216,8 @@ 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 = SignOutFromOrDeleteTeamsByID(s, u, utils.NotIn(oldOidcTeams, oidcTeams))
errs = SignOutFromTeamsByID(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)
}
@ -254,27 +255,23 @@ 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 SignOutFromOrDeleteTeamsByID(s *xorm.Session, u *user.User, teamIDs []int64) (errs []error) {
func SignOutFromTeamsByID(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}
exists, _ := tm.CheckMembership(s)
memberCount, _ := tm.GetMemberCount(s)
exists, err := tm.CheckMembership(s)
if err != nil {
errs = append(errs, err)
continue
}
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-02-13 10:31:46 +00:00
Outdated
Review
Copy Link

This whole inner loop should be rewritten to something like this so that it's more idomatic go:

tm := models.TeamMember{TeamID: teamID, Username: u.Username}
err := tm.Delete(s, u)
if err != nil {
	return err
}

team, err := models.GetTeamByID(s, teamID)
if err != nil {
	return err
}

err = team.Delete(s, u)
if err != nil {
	return err
}
This whole inner loop should be rewritten to something like this so that it's more idomatic go: ```go tm := models.TeamMember{TeamID: teamID, Username: u.Username} err := tm.Delete(s, u) if err != nil { return err } team, err := models.GetTeamByID(s, teamID) if err != nil { return err } err = team.Delete(s, u) if err != nil { return err }
konrad commented 2023-02-13 10:32:42 +00:00
Outdated
Review
Copy Link

Also right now you'll only delete the team if deleting the member did not work, which does not really make sense.

Also right now you'll only delete the team if deleting the member did not work, which does not really make sense.
viehlieb commented 2023-02-23 14:58:37 +00:00
Outdated
Review
Copy Link

changed behaviour to not delete team

changed behaviour to not delete team
if !exists {
continue
}
err := tm.Delete(s, u)
err = tm.Delete(s, u)
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-02-23 15:32:48 +00:00
Outdated
Review
Copy Link

Not sure if this is a good solution. Imagine one membership fails to get removed while the others get through. Then there's inconsistent state and the whole thing is not atomic anymore. I think we should instead return on the first error and let the caller deal with it.

Not sure if this is a good solution. Imagine one membership fails to get removed while the others get through. Then there's inconsistent state and the whole thing is not atomic anymore. I think we should instead return on the first error and let the caller deal with it.
viehlieb commented 2023-03-10 12:19:46 +00:00
Outdated
Review
Copy Link

okay, but won't it reach inconsistent state anyways?
Imagine the user is in 3 groups.
tm is deleted for group 1
tm is not deleted for group 2, because user is last member
tm won't be deleted for group 3, where user is not last member and should be removed.

or do you suggest to catch ErrCannotDeleteLastTeamMember and ask to sign out manually from that specific group(s) and sign out from other groups anyway?

I think it is intended behavior to sign out from teams when possible, and to leave teams as they are, where error is found. Maybe giving a hint to leave manually because you are last user.

Did I understand you correctly?

okay, but won't it reach inconsistent state anyways? Imagine the user is in 3 groups. tm is deleted for group 1 tm is not deleted for group 2, because user is last member tm won't be deleted for group 3, where user is not last member and should be removed. or do you suggest to catch ErrCannotDeleteLastTeamMember and ask to sign out manually from that specific group(s) and sign out from other groups anyway? I think it is intended behavior to sign out from teams when possible, and to leave teams as they are, where error is found. Maybe giving a hint to leave manually because you are last user. Did I understand you correctly?
konrad commented 2023-03-10 16:01:29 +00:00
Outdated
Review
Copy Link

I think it is intended behavior to sign out from teams when possible, and to leave teams, where error is found.

What's your difference between "signing out from a team" and "leaving a team"? Aren't the two the same?

okay, but won't it reach inconsistent state anyways?

Not if the whole db transaction is rolled back.

> I think it is intended behavior to sign out from teams when possible, and to leave teams, where error is found. What's your difference between "signing out from a team" and "leaving a team"? Aren't the two the same? > okay, but won't it reach inconsistent state anyways? Not if the whole db transaction is rolled back.
viehlieb commented 2023-03-10 16:23:47 +00:00
Outdated
Review
Copy Link

edited

edited
viehlieb commented 2023-04-17 12:40:17 +00:00
Outdated
Review
Copy Link

the edit is:

I think it is intended behavior to sign out from teams when possible, and to leave teams, where error is found.

What's your difference between "signing out from a team" and "leaving a team"? Aren't the two the same?

I think it is intended behavior to sign out from teams when possible, and to leave teams as they are, where error is found. Maybe giving a hint to leave manually because you are last user.

okay, but won't it reach inconsistent state anyways?

Not if the whole db transaction is rolled back.

If the whole transaction is rolled back, then it might very challening to organize teams, because if a member sits on a team as last member, then the member cannot leave this team.
The suggestion would be to leave teams, if it is possible -> the team membership, where the user cannot be signed out because of
ErrCannotDeleteLastTeamMember{tm.TeamID, tm.UserID}
should be resolved manually then

the edit is: > > I think it is intended behavior to sign out from teams when possible, and to leave teams, where error is found. > > What's your difference between "signing out from a team" and "leaving a team"? Aren't the two the same? > I think it is intended behavior to sign out from teams when possible, and to leave teams **as they are**, where error is found. Maybe giving a hint to leave manually because you are last user. > > okay, but won't it reach inconsistent state anyways? > Not if the whole db transaction is rolled back. If the whole transaction is rolled back, then it might very challening to organize teams, because if a member sits on a team as last member, then the member **cannot leave this** team. The suggestion would be to leave teams, if it is possible -> the team membership, where the user cannot be signed out because of ```ErrCannotDeleteLastTeamMember{tm.TeamID, tm.UserID}``` should be resolved manually then
konrad commented 2023-04-17 16:14:56 +00:00
Outdated
Review
Copy Link

to leave teams as they are, where error is found

Ah so "leave a team" in this context is "not deleting / modifying it"? Not "A user leaves a team"?

I guess I'm just understanding it differently :)

if a member sits on a team as last member, then the member cannot leave other teams.

Why would it do that? That's not what should happen, but how would being the last member of one team prevent that user to leave another team?

where the user cannot be signed out because of
ErrCannotDeleteLastTeamMember{tm.TeamID, tm.UserID}
should be resolved manually then

But resolving this manually would mean the user needs to manually remove the team, which they cannot do because the team was created from openid?

> to leave teams as they are, where error is found Ah so "leave a team" in this context is "not deleting / modifying it"? Not "A user leaves a team"? I guess I'm just understanding it differently :) > if a member sits on a team as last member, then the member cannot leave other teams. Why would it do that? That's not what should happen, but how would being the last member of one team prevent that user to leave another team? > where the user cannot be signed out because of ErrCannotDeleteLastTeamMember{tm.TeamID, tm.UserID} should be resolved manually then But resolving this manually would mean the user needs to manually remove the team, which they cannot do because the team was created from openid?
viehlieb commented 2023-11-07 13:41:23 +00:00
Outdated
Review
Copy Link

if a member sits on a team as last member, then the member cannot leave other teams.
...is edited. I meant: the member cannot leave this team, if it is the last membership.

where the user cannot be signed out because of
ErrCannotDeleteLastTeamMember{tm.TeamID, tm.UserID}
should be resolved manually then

But resolving this manually would mean the user needs to manually remove the team, which they cannot do because the team was created from openid?

I think it is the responsibility of oidc admins to not let a team go blank without coordinating the potential destructive danger of it.
Therefore i would just delete the team + team_memberships.

> if a member sits on a team as last member, then the member cannot leave other teams. ...is edited. I meant: the member cannot leave this team, if it is the last membership. > where the user cannot be signed out because of ErrCannotDeleteLastTeamMember{tm.TeamID, tm.UserID} should be resolved manually then > But resolving this manually would mean the user needs to manually remove the team, which they cannot do because the team was created from openid? I think it is the responsibility of _oidc admins_ to not let a team go blank without coordinating the potential destructive danger of it. Therefore i would just delete the team + team_memberships.
konrad commented 2023-11-07 15:02:36 +00:00
Outdated
Review
Copy Link

I think it is the responsibility of oidc admins to not let a team go blank without coordinating the potential destructive danger of it. Therefore i would just delete the team + team_memberships.

Okay, that makes more sense.

> I think it is the responsibility of oidc admins to not let a team go blank without coordinating the potential destructive danger of it. Therefore i would just delete the team + team_memberships. Okay, that makes more sense.
👍 1
// if you cannot delete the team_member
if err != nil || memberCount <= 1 {
team, err := models.GetTeamByID(s, teamID)
if err != nil {
errs = append(errs, err)
continue
}
err = team.Delete(s, u)
if err != nil {
errs = append(errs, err)
}
if err != nil {
errs = append(errs, err)
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-02-13 10:33:56 +00:00
Outdated
Review
Copy Link

Why not cast this to []map[string]interface{} directly?

Why not cast this to `[]map[string]interface{}` directly?
viehlieb commented 2023-02-23 15:18:41 +00:00
Outdated
Review
Copy Link

no reason,
had claim VikunjaGroups more agnostic to specific data type.
now casts directly to []map[string]interface{}

no reason, had claim VikunjaGroups more agnostic to specific data type. now casts directly to []map[string]interface{}
continue
}
}
return errs