feat: assign users to teams via OIDC claims #1393
|
@ -203,23 +203,24 @@ func HandleCallback(c echo.Context) error {
|
|||
|
||||
viehlieb marked this conversation as resolved
Outdated
|
||||
// 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
Typo: 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
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
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
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..
|
||||
err = s.Commit()
|
||||
if err != nil {
|
||||
|
@ -232,29 +233,30 @@ func HandleCallback(c echo.Context) error {
|
|||
}
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
This should be an This should be an `Error` log message.
|
||||
|
||||
func AssignOrCreateUserToTeams(s *xorm.Session, u *user.User, teamData []models.OIDCTeamData) (oidcTeams []int64, err error) {
|
||||
if len(teamData) > 0 {
|
||||
// check if we have seen these teams before.
|
||||
// find or create Teams and assign user as teammember.
|
||||
teams, err := GetOrCreateTeamsByOIDCAndNames(s, teamData, u)
|
||||
if err != nil {
|
||||
log.Errorf("Error verifying team for %v, got %v. Error: %v", u.Name, teams, err)
|
||||
return nil, err
|
||||
}
|
||||
for _, team := range teams {
|
||||
tm := models.TeamMember{TeamID: team.ID, UserID: u.ID, Username: u.Username}
|
||||
exists, _ := tm.CheckMembership(s)
|
||||
if !exists {
|
||||
err = tm.Create(s, u)
|
||||
if err != nil {
|
||||
log.Errorf("Could not assign %v to %v. %v", u.Username, team.Name, err)
|
||||
}
|
||||
if len(teamData) == 0 {
|
||||
return
|
||||
}
|
||||
// check if we have seen these teams before.
|
||||
// find or create Teams and assign user as teammember.
|
||||
teams, err := GetOrCreateTeamsByOIDCAndNames(s, teamData, u)
|
||||
if err != nil {
|
||||
log.Errorf("Error verifying team for %v, got %v. Error: %v", u.Name, teams, err)
|
||||
return nil, err
|
||||
}
|
||||
for _, team := range teams {
|
||||
tm := models.TeamMember{TeamID: team.ID, UserID: u.ID, Username: u.Username}
|
||||
exists, _ := tm.CheckMembership(s)
|
||||
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.
|
||||
if !exists {
|
||||
err = tm.Create(s, u)
|
||||
if err != nil {
|
||||
log.Errorf("Could not assign %v to %v. %v", u.Username, team.Name, err)
|
||||
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
|
||||
}
|
||||
oidcTeams = append(oidcTeams, team.ID)
|
||||
}
|
||||
oidcTeams = append(oidcTeams, team.ID)
|
||||
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.
|
||||
}
|
||||
return oidcTeams, err
|
||||
|
||||
}
|
||||
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
```
|
||||
|
||||
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
|
||||
func RemoveUserFromTeamsByIds(s *xorm.Session, u *user.User, teamIDs []int64) (errs []error) {
|
||||
errs = []error{}
|
||||
for _, teamID := range teamIDs {
|
||||
|
@ -313,19 +315,19 @@ func getTeamDataFromToken(groups []map[string]interface{}, provider *Provider) (
|
|||
}
|
||||
|
||||
func CreateTeamWithData(s *xorm.Session, teamData models.OIDCTeamData, u *user.User) (team *models.Team, err error) {
|
||||
tea := &models.Team{
|
||||
team = &models.Team{
|
||||
Name: teamData.TeamName,
|
||||
Description: teamData.Description,
|
||||
OidcID: teamData.OidcID,
|
||||
}
|
||||
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{}`.
|
||||
err = tea.Create(s, u)
|
||||
return tea, err
|
||||
err = team.Create(s, u)
|
||||
return team, err
|
||||
}
|
||||
viehlieb marked this conversation as resolved
Outdated
konrad
commented
Which oidcID? In the Which oidcID? In the `teamData`? Isn't that always a string, due to the type system? Then the comment is redundant, please remove or clarify further.
|
||||
|
||||
// this functions creates an array of existing teams that was generated from the oidc data.
|
||||
func GetOrCreateTeamsByOIDCAndNames(s *xorm.Session, teamData []models.OIDCTeamData, u *user.User) (te []*models.Team, err error) {
|
||||
te = []*models.Team{}
|
||||
// Procedure can only be successful if oidcID is set and converted to string
|
||||
// Procedure can only be successful if oidcID is set
|
||||
for _, oidcTeam := range teamData {
|
||||
team, err := models.GetTeamByOidcIDAndName(s, oidcTeam.OidcID, oidcTeam.TeamName)
|
||||
if err != nil {
|
||||
|
|
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.
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.