feat: assign users to teams via OIDC claims #1393
|
@ -58,11 +58,6 @@ type Provider struct {
|
|||
openIDProvider *oidc.Provider
|
||||
Oauth2Config *oauth2.Config `json:"-"`
|
||||
}
|
||||
type TeamData struct {
|
||||
TeamName string
|
||||
OidcID string
|
||||
Description string
|
||||
}
|
||||
type claims struct {
|
||||
Email string `json:"email"`
|
||||
viehlieb marked this conversation as resolved
Outdated
|
||||
Name string `json:"name"`
|
||||
|
@ -269,8 +264,8 @@ func SignOutFromOrDeleteTeamsByID(s *xorm.Session, u *user.User, teamIDs []int64
|
|||
}
|
||||
}
|
||||
|
||||
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
|
||||
func getTeamDataFromToken(groups interface{}, provider *Provider) (teamData []TeamData, err error) {
|
||||
teamData = []TeamData{}
|
||||
func getTeamDataFromToken(groups interface{}, provider *Provider) (teamData []models.TeamData, err error) {
|
||||
teamData = []models.TeamData{}
|
||||
if groups != nil {
|
||||
el := groups.([]interface{})
|
||||
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.
|
||||
for _, data := range el {
|
||||
|
@ -301,13 +296,13 @@ func getTeamDataFromToken(groups interface{}, provider *Provider) (teamData []Te
|
|||
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})
|
||||
teamData = append(teamData, models.TeamData{TeamName: name, OidcID: oidcID, Description: description})
|
||||
}
|
||||
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
|
||||
}
|
||||
return teamData, nil
|
||||
}
|
||||
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.
|
||||
|
||||
func CreateTeamWithData(s *xorm.Session, teamData TeamData, u *user.User) (team *models.Team, err error) {
|
||||
func CreateTeamWithData(s *xorm.Session, teamData models.TeamData, u *user.User) (team *models.Team, err error) {
|
||||
tea := &models.Team{
|
||||
Name: teamData.TeamName,
|
||||
Description: teamData.Description,
|
||||
|
@ -320,7 +315,7 @@ func CreateTeamWithData(s *xorm.Session, teamData TeamData, u *user.User) (team
|
|||
}
|
||||
|
||||
// this functions creates an array of existing teams that was generated from the oidc data.
|
||||
func GetOrCreateTeamsByOIDCAndNames(s *xorm.Session, teamData []TeamData, u *user.User) (te []models.Team, err error) {
|
||||
func GetOrCreateTeamsByOIDCAndNames(s *xorm.Session, teamData []models.TeamData, u *user.User) (te []models.Team, err error) {
|
||||
te = []models.Team{}
|
||||
// Procedure can only be successful if oidcID is set and converted to string
|
||||
for _, oidcTeam := range teamData {
|
||||
|
|
Didn't you declare this already somewhere else?