feat: assign users to teams via OIDC claims #1393
|
@ -152,6 +152,7 @@ func HandleCallback(c echo.Context) error {
|
|||
|
||||
// Extract custom claims
|
||||
cl := &claims{}
|
||||
|
||||
err = idToken.Claims(cl)
|
||||
if err != nil {
|
||||
log.Errorf("Error getting token claims for provider %s: %v", provider.Name, err)
|
||||
|
@ -206,57 +207,117 @@ func HandleCallback(c echo.Context) error {
|
|||
return handler.HandleHTTPError(err, c)
|
||||
}
|
||||
|
||||
// Check if we have seen these teams before
|
||||
if len(cl.Teams) > 0 {
|
||||
teams, err := GetOrCreateTeamsByNames(s, cl.Teams, u)
|
||||
// does the oidc token contain well formed "vikunja_groups" through vikunja_scope
|
||||
viehlieb marked this conversation as resolved
Outdated
|
||||
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)
|
||||
}
|
||||
// check if we have seen these teams before.
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
Please don't ignore the error. Please don't ignore the error.
|
||||
// find or create Teams and assign user as teammember.
|
||||
if len(teamData) > 0 {
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
Please don't call this "Sign out". That's a different thing. Please don't call this "Sign out". That's a different thing.
|
||||
log.Debugf("TeamData is set %v", teamData)
|
||||
teams, err := GetOrCreateTeamsByNames(s, teamData, u)
|
||||
if err != nil {
|
||||
log.Errorf("Error verifying team for name %v, got %v", cl.Name, teams, err)
|
||||
return err
|
||||
} else {
|
||||
for _, team := range teams {
|
||||
tm := models.TeamMember{TeamID: team.ID, Username: u.Username}
|
||||
err := tm.CheckMembership(s)
|
||||
if err == nil {
|
||||
tm.Create(s, u)
|
||||
}
|
||||
konrad marked this conversation as resolved
Outdated
konrad
commented
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
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..
|
||||
|
||||
for _, team := range teams {
|
||||
tm := models.TeamMember{TeamID: team.ID, Username: u.Username}
|
||||
exists, err := tm.CheckMembership(s)
|
||||
if !exists {
|
||||
err = tm.Create(s, u)
|
||||
if err != nil {
|
||||
log.Debugf("Could not assign %v to %v. %v", u.Username, team.Name, err)
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
Replace with
Replace with
```
if len(teamData) < 1 {
return
}
viehlieb
commented
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
But I should have phrased it better, this is equivalent:
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
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.
|
||||
}
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
This should be an This should be an `Error` log message.
|
||||
} else {
|
||||
log.Debugf("Team exists? %v or error: %v", exists, err)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
err = s.Commit()
|
||||
if err != nil {
|
||||
_ = s.Rollback()
|
||||
log.Errorf("Error creating new Team for provider %s: %v", provider.Name, err)
|
||||
return handler.HandleHTTPError(err, c)
|
||||
}
|
||||
// Create token
|
||||
return auth.NewUserAuthTokenResponse(u, c, false)
|
||||
}
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
Please change this error message to 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.
|
||||
|
||||
func GetOrCreateTeamsByNames(s *xorm.Session, teamNames []string, u *user.User) (te []models.Team, err error) {
|
||||
te = []models.Team{}
|
||||
for _, t := range teamNames {
|
||||
team, err := models.GetTeamsByName(s, t)
|
||||
|
||||
// if team does not exists, create it
|
||||
if models.IsErrTeamsDoNotExist(err) {
|
||||
log.Debugf("No such Team: %v, create %v ..", t, team)
|
||||
tea := &models.Team{
|
||||
Name: t,
|
||||
func getTeamDataFromToken(groups interface{}, provider *Provider) (teamData []TeamData, err error) {
|
||||
teamData = []TeamData{}
|
||||
if groups != nil {
|
||||
konrad marked this conversation as resolved
Outdated
konrad
commented
What does this do? Ensure the user is only member of teams provided by the oidc provider? What about teams created in Vikunja manually? I feel like we should not delete a team here as that's pretty destructive. Maybe add a command to clean up empty teams later but I think we should not do that here. What does this do? Ensure the user is only member of teams provided by the oidc provider? What about teams created in Vikunja manually?
I feel like we should not delete a team here as that's pretty destructive. Maybe add a command to clean up empty teams later but I think we should not do that here.
viehlieb
commented
User as "team_member" will be removed via Further explanation below: deleting the team is not managed via oidc. Meaning:
what is missing though: User as "team_member" will be removed via `(user.id, team.id)` for all `teamIDs []int64`.
If the user is the last member, the team is deleted.
Further explanation below:
deleting the team is not managed via oidc.
the token delivered by the oidc provider just contains information about the team memberships of the user.
### Meaning:
* `team1` exists and was created through oidc (oidcId is set to something not null or "").
* `user1` is team_member of team1.
* changes in oidc provider change team_membership of `user1` >> `team1` is not longer contained in oidc token for `user1`.
* `user1` logs in through oidc and is kicked out of team, which is not possible, if `user1` is the last member of `team1`.
* if `user1` is last member >> `team1` is deleted.
what is missing though:
a check whether `user1` is in fact the last member of `team1`, then only delete `team1`.
konrad
commented
Ah, now I understand how that came about. I still think we should not delete the team at that moment, better change the function to allow removing the last member in that case. There will probably be cases where a user is accidentally removed from the oicd claim, the team gets deleted and all shares with that team as well. Then the user is added again, and the shares need to be added again. It's fine to use this to sync teams and memberships but I feel like there's more to it. > user1 logs in through oidc and is kicked out of team, which is not possible, if user1 is the last member of team1.
Ah, now I understand how that came about. I still think we should not delete the team at that moment, better change the function to allow removing the last member in that case. There will probably be cases where a user is accidentally removed from the oicd claim, the team gets deleted and all shares with that team as well. Then the user is added again, and the shares need to be added again.
I don't know if this is only hypothetical, but it's something users will complain about when it happens.
It's fine to use this to sync teams and memberships but I feel like there's more to it.
viehlieb
commented
okay, teams won't be deleted okay, teams won't be deleted
|
||||
el := groups.([]interface{})
|
||||
for _, data := range el {
|
||||
team := data.(map[string]interface{})
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
Please call this something like Please call this something like `RemoveUserFromTeamsByID` so that it is clear how destructive this is.
|
||||
log.Debugf("%s", team)
|
||||
var name string
|
||||
var description string
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
why not do this in one sql query? why not do this in one sql query?
viehlieb
commented
You mean something like:
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
Yes, but with xorm:
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
went for:
went for:
`_, err = s.Where("team_id IN (?) AND user_id = ?", strings.Join(strSlice, ","), u.ID).Delete(&models.TeamMember{})`
konrad
commented
That's still working agains the orm - makes things more complicated in the end. Please use xorm's Something like this might work as well:
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
```
|
||||
var oidcID string
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
Please return the error instead of logging only. Please return the error instead of logging only.
konrad
commented
Please don't ignore the errors. Please don't ignore the errors.
viehlieb
commented
not ignored now not ignored now
|
||||
if team["name"] != nil {
|
||||
name = team["name"].(string)
|
||||
}
|
||||
err := tea.Create(s, u)
|
||||
if err != nil {
|
||||
log.Errorf("Teams: %v, err: %v", tea, err)
|
||||
} else {
|
||||
te = append(te, *tea)
|
||||
if team["description"] != nil {
|
||||
description = team["description"].(string)
|
||||
}
|
||||
} else {
|
||||
// if multiple teams with same name are found,
|
||||
if len(team) == 1 {
|
||||
// append team to return value
|
||||
te = append(te, *team[len(team)-1])
|
||||
} else {
|
||||
log.Debugf("Multiple Teams have the same name: %v, ignore assignment of %v", team[len(team)-1].Name, u.Name)
|
||||
if team["oidcID"] != nil {
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
This whole inner loop should be rewritten to something like this so that it's more idomatic go:
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
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
changed behaviour to not delete team changed behaviour to not delete team
|
||||
switch t := team["oidcID"].(type) {
|
||||
case int64:
|
||||
oidcID = fmt.Sprintf("%v", team["oidcID"])
|
||||
case string:
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
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
okay, but won't it reach inconsistent state anyways? 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
What's your difference between "signing out from a team" and "leaving a team"? Aren't the two the same?
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
edited edited
viehlieb
commented
the edit is:
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.
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 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
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 :)
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?
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
I think it is the responsibility of oidc admins to not let a team go blank without coordinating the potential destructive danger of it. > 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
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.
|
||||
oidcID = string(team["oidcID"].(string))
|
||||
case float64:
|
||||
fl := float64(team["oidcID"].(float64))
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
Why not cast this to Why not cast this to `[]map[string]interface{}` directly?
viehlieb
commented
no reason, no reason,
had claim VikunjaGroups more agnostic to specific data type.
now casts directly to []map[string]interface{}
|
||||
oidcID = fmt.Sprintf("%v", fl)
|
||||
default:
|
||||
log.Errorf("No oidcID assigned for %v or type %v not supported", team, t)
|
||||
}
|
||||
}
|
||||
if name == "" || oidcID == "" {
|
||||
log.Errorf("Claim of your custom scope does not hold name or oidcID for automatic group assignment through oidc provider. Please check %s", provider.Name)
|
||||
return teamData, &user.ErrOpenIDCustomScopeMalformed{}
|
||||
}
|
||||
teamData = append(teamData, TeamData{TeamName: name, OidcID: oidcID, Description: description})
|
||||
}
|
||||
}
|
||||
return teamData, nil
|
||||
}
|
||||
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
Please use `strconv.FormatInt Please use `strconv.FormatInt
viehlieb
commented
done done
|
||||
func CreateTeamWithData(s *xorm.Session, teamData TeamData, u *user.User) (team *models.Team, err error) {
|
||||
tea := &models.Team{
|
||||
Name: teamData.TeamName,
|
||||
Description: teamData.Description,
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
Please use xorm's 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.
|
||||
OidcID: teamData.OidcID,
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
Please use Please use `strconv.FormatFloat` (not 100% sure if the function is really called that)
|
||||
}
|
||||
log.Debugf("Teams: %v", tea.OidcID)
|
||||
|
||||
err = tea.Create(s, u)
|
||||
return tea, err
|
||||
}
|
||||
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
The return here will mean if one team claim is invalid, it will not try to validate any others. Is that what you intended? The return here will mean if one team claim is invalid, it will not try to validate any others. Is that what you intended?
viehlieb
commented
changed behaviour to skipping the invalid team changed behaviour to skipping the invalid team
|
||||
// this functions creates an array of existing teams that was generated from the oidc data.
|
||||
func GetOrCreateTeamsByNames(s *xorm.Session, teamData []TeamData, u *user.User) (te []models.Team, err error) {
|
||||
te = []models.Team{}
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
Please use Please use `_, exists := team["name"]` to check if the field exists (`exists` is a boolean indicating whether the field exists). Same for the other fields.
|
||||
// Procedure can only be successful if oidcID is set and converted to string
|
||||
for _, oidcTeam := range teamData {
|
||||
team, err := models.GetTeamByOidcIDAndName(s, oidcTeam.OidcID, oidcTeam.TeamName)
|
||||
if err != nil {
|
||||
log.Debugf("Team with oidc_id %v and name %v does not exist. Create Team.. ", oidcTeam.OidcID, oidcTeam.TeamName)
|
||||
newTeam, err := CreateTeamWithData(s, oidcTeam, u)
|
||||
if err != nil {
|
||||
return te, err
|
||||
}
|
||||
te = append(te, *newTeam)
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
Please rename this to Please rename this to `team`.
konrad
commented
What's the advantage of this method over What's the advantage of this method over `team.Create`?
viehlieb
commented
i think it was cyclomatic complexity. i think it was cyclomatic complexity.
|
||||
} else {
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
Please make this debug message more descriptive. Please make this debug message more descriptive.
|
||||
log.Debugf("Team with oidc_id %v and name %v already exists.", team.OidcID, team.Name)
|
||||
te = append(te, team)
|
||||
}
|
||||
|
||||
}
|
||||
log.Debugf("Array: %v", te)
|
||||
return te, err
|
||||
}
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
Please use Please use
viehlieb
commented
Here there is the information missing, what do you suggest? Here there is the information missing, what do you suggest?
konrad
commented
Sorry, I meant Sorry, I meant `[]*models.Team{}`.
|
||||
|
||||
|
|
Typo:
team
andoidc
should be lowercase.Please check the other log messages as well for typos, I've seen a few more like this.