vikunja/vikunja
6
43
Fork 73
Code Issues 150 Pull Requests 18 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 +23 -16
1 changed files with 23 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 46a6456bae - Show all commits

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

@ -203,23 +203,24 @@ func HandleCallback(c echo.Context) error {
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-02-23 15:30:09 +00:00
Outdated
Review
Copy Link

I feel like the next 20 lines need better error handling - is it intended to not return when an error happens?

I feel like the next 20 lines need better error handling - is it intended to not return when an error happens?
viehlieb commented 2023-03-10 12:30:40 +00:00
Outdated
Review
Copy Link

The rationale was to ensure users can still log in, even though the scope is malformed.
Therefore the Errors are logged instead of returning.

Also: It might be some groups with valid token details get through, some not.

We can also go for not signing in altogether.

The rationale was to ensure users can still log in, even though the scope is malformed. Therefore the Errors are logged instead of returning. Also: It might be some groups with valid token details get through, some not. We can also go for not signing in altogether.
konrad commented 2023-03-10 15:58:58 +00:00
Outdated
Review
Copy Link

If this works and won't create duplicate teams when users log in again in the future, I think it's fine to leave it like this.

If this works and won't create duplicate teams when users log in again in the future, I think it's fine to leave it like this.
// does the oidc token contain well formed "vikunja_groups" through vikunja_scope
teamData, errs := getTeamDataFromToken(cl.VikunjaGroups, provider)
for _, err := range errs {
log.Errorf("Error creating teams for user and vikunja groups %s: %v", cl.VikunjaGroups, err)
}
if teamData != nil {
for _, err := range errs {
log.Errorf("Error creating teams for user and vikunja groups %s: %v", cl.VikunjaGroups, err)
}
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-10-18 15:18:17 +00:00
Outdated
Review
Copy Link

Typo: team and oidc should be lowercase.

Please check the other log messages as well for typos, I've seen a few more like this.

Typo: `team` and `oidc` should be lowercase. Please check the other log messages as well for typos, I've seen a few more like this.
//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))
log.Errorf("%v", errs)
for _, err := range errs {
log.Errorf("Found Error while signing out from teams %v", 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)
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.
if err != nil {
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 = RemoveUserFromTeamsByIds(s, u, utils.NotIn(oldOidcTeams, oidcTeams))
for _, err := range errs {
log.Errorf("Found Error while signing out from teams %v", err)
}
}
konrad marked this conversation as resolved Outdated
konrad commented 2023-12-01 14:59:58 +00:00
Outdated
Review
Copy Link

Doing this every time a user logs in probably has performance implications but that's a problem to care about when we'll hit it in the wild.

Doing this every time a user logs in probably has performance implications but that's a problem to care about when we'll hit it in the wild.
viehlieb commented 2023-12-06 13:37:36 +00:00
Outdated
Review
Copy Link

only if it has teamIDsToLeave, but yeah I agree.

The problem is more the oidc structure itself, that you "have" to check for changes "everytime" you sign in..

only if it has teamIDsToLeave, but yeah I agree. The problem is more the oidc structure itself, that you "have" to check for changes "everytime" you sign in..
err = s.Commit()
if err != nil {
@ -254,8 +255,10 @@ func AssignOrCreateUserToTeams(s *xorm.Session, u *user.User, teamData []models.
for _, err := range errs {
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.
log.Errorf("Found Error while signing out from teams %v", err)
}
oidcTeams = append(oidcTeams, team.ID)
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 ```
}
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
return oidcTeams, err
<<<<<<< HEAD
func AssignOrCreateUserToTeams(s *xorm.Session, u *user.User, teamData []models.OIDCTeamData) (oidcTeams []int64, err error) {
if len(teamData) == 0 {
@ -285,6 +288,10 @@ func AssignOrCreateUserToTeams(s *xorm.Session, u *user.User, teamData []models.
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-02-13 10:35:31 +00:00
Outdated
Review
Copy Link

Please use `strconv.FormatInt

Please use `strconv.FormatInt
viehlieb commented 2023-02-23 14:25:52 +00:00
Outdated
Review
Copy Link

done

done
=======
>>>>>>> 169b668c... remove left over function GetMemberCount, rename function SignOut to RemoveFrom
=======
}
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-12-01 15:01:28 +00:00
Outdated
Review
Copy Link

Please use xorm's In function because that allows you to pass an int slice directly and avoids the need for the conversion you're doing.

Please use xorm's `In` function because that allows you to pass an int slice directly and avoids the need for the conversion you're doing.
viehlieb marked this conversation as resolved Outdated
konrad commented 2023-02-13 10:35:56 +00:00
Outdated
Review
Copy Link

Please use strconv.FormatFloat (not 100% sure if the function is really called that)

Please use `strconv.FormatFloat` (not 100% sure if the function is really called that)
>>>>>>> 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 {