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
2 changed files with 22 additions and 28 deletions
Showing only changes of commit 105df8588e - Show all commits

View File

@ -115,6 +115,15 @@ 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.
viehlieb marked this conversation as resolved Outdated

Couldn't you just return the result here directly?

Couldn't you just return the result here directly?

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.
Where("team_id = ?", tm.TeamID).
Cols("user_id").
Find(&members)
viehlieb marked this conversation as resolved Outdated

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

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

done

done
return len(members), 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.

View File

@ -195,7 +195,6 @@ func HandleCallback(c echo.Context) error {
// Check if we have seen this user before
u, err := getOrCreateUser(s, cl, idToken.Issuer, idToken.Subject)
if err != nil {
_ = s.Rollback()
log.Errorf("Error creating new user for provider %s: %v", provider.Name, err)
@ -203,14 +202,18 @@ func HandleCallback(c echo.Context) error {
}
viehlieb marked this conversation as resolved Outdated

The next 20 lines should be wrapped in a if teamData != nil so that the whole thing only gets executed when there is team data in the token. Otherwise this might generate error messages for setups not using teams via oicd.

The next 20 lines should be wrapped in a `if teamData != nil` so that the whole thing only gets executed when there is team data in the token. Otherwise this might generate error messages for setups not using teams via oicd.

i removed this because of "cyclomatic complexity". But i can try to put it back in.

i removed this because of "cyclomatic complexity". But i can try to put it back in.

Well if the cyclomatic complexity gets too high the fix would be to refactor this into multiple functions, not to remove the check :) because the check should be there.

Well if the cyclomatic complexity gets too high the fix would be to refactor this into multiple functions, not to remove the check :) because the check should be there.
viehlieb marked this conversation as resolved Outdated

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?

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.

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, err := getTeamDataFromToken(cl.VikunjaGroups, provider)
if err != nil {
log.Errorf("Error creating teams for user and vikunja groups %s: %v", cl.VikunjaGroups, err)
return handler.HandleHTTPError(err, c)
teamData, errs := getTeamDataFromToken(cl.VikunjaGroups, provider)
if len(errs) > 0 {
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

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, _ := models.FindAllOidcTeamIDsForUser(s, u.ID)
oldOidcTeams, err := models.FindAllOidcTeamIDsForUser(s, u.ID)
if err != nil {
log.Errorf("No Oidc Teams found for user", err)
}
viehlieb marked this conversation as resolved Outdated

Please don't ignore the error.

Please don't ignore the error.
var oidcTeams []int64
if len(teamData) > 0 {
viehlieb marked this conversation as resolved Outdated

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

Please don't call this "Sign out". That's a different thing.
// check if we have seen these teams before.
@ -236,7 +239,10 @@ func HandleCallback(c echo.Context) error {
log.Errorf("Found Error while signing out from teams %v", err)
}
}
SignOutFromOrDeleteTeamsByID(s, u, notIn(oldOidcTeams, oidcTeams))
errs = SignOutFromOrDeleteTeamsByID(s, u, utils.NotIn(oldOidcTeams, oidcTeams))
for _, err := range errs {
log.Errorf("Found Error while signing out from teams %v", err)
}
err = s.Commit()
if err != nil {
_ = s.Rollback()
viehlieb marked this conversation as resolved Outdated

Please change this error message to Could not assign user %s to team %s: %v - we also have assignees so this could be confusing on its own.

Please change this error message to `Could not assign user %s to team %s: %v` - we also have assignees so this could be confusing on its own.
@ -358,7 +364,6 @@ func GetOrCreateTeamsByOIDCAndNames(s *xorm.Session, teamData []models.OIDCTeamD
}
}
log.Debugf("Array: %v", te)
return te, err
}
@ -429,23 +434,3 @@ func getOrCreateUser(s *xorm.Session, cl *claims, issuer, subject string) (u *us
return
}
// find the elements which appear in slice1,but not in slice2
func notIn(slice1 []int64, slice2 []int64) []int64 {
var diff []int64
for _, s1 := range slice1 {
found := false
for _, s2 := range slice2 {
if s1 == s2 {
found = true
break
}
}
// String not found. We add it to return slice
if !found {
diff = append(diff, s1)
}
}
return diff
}