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 124 additions and 6 deletions
Showing only changes of commit 1123d7b98c - Show all commits

View File

@ -79,7 +79,7 @@ type TeamMember struct {
}
// TableName makes beautiful table names
func (*TeamMember) TableName() string {
func (TeamMember) TableName() string {
return "team_members"
}
viehlieb marked this conversation as resolved Outdated

Why did you change this?

Why did you change this?

oha.
this was unintended.

oha. this was unintended.
@ -119,6 +119,34 @@ func GetTeamByID(s *xorm.Session, id int64) (team *Team, err error) {
return
}
func GetTeamsByName(s *xorm.Session, name string) (teams []*Team, err error) {
if name == "" {
return teams, ErrTeamsDoNotExist{name}
}
var ts []*Team
exists := s.
Where("name = ?", name).
Find(&ts)
if exists != nil {
return
}
viehlieb marked this conversation as resolved Outdated

Please return a pointer to a

Please return a pointer to a

team i suppose.done, tx

team i suppose.done, tx
if len(ts) == 0 {
return ts, ErrTeamsDoNotExist{name}
}
// //for each ts
konrad marked this conversation as resolved Outdated

Did you try passing the

Did you try passing the

Ups, I guess here is something missing

Ups, I guess here is something missing
// teamSlice := []*Team{ts}
// err = addMoreInfoToTeams(s, teamSlice)
viehlieb marked this conversation as resolved Outdated

Get will always return one entry, no need for Asc or Limit.

`Get` will always return one entry, no need for `Asc` or `Limit`.
// if err != nil {
// return
// }
teams = ts
return
}
func addMoreInfoToTeams(s *xorm.Session, teams []*Team) (err error) {
@ -282,6 +310,37 @@ func (t *Team) Create(s *xorm.Session, a web.Auth) (err error) {
})
}
func (t *Team) CreateNoAdmin(s *xorm.Session, a web.Auth) (err error) {
doer, err := user.GetFromAuth(a)
if err != nil {
return err
}
// Check if we have a name
if t.Name == "" {
return ErrTeamNameCannotBeEmpty{}
}
t.CreatedByID = doer.ID
t.CreatedBy = doer
_, err = s.Insert(t)
if err != nil {
return
viehlieb marked this conversation as resolved Outdated

Why the extra variable?

Why the extra variable?

manageAdmin func and this var are deleted from code, since not needed

manageAdmin func and this var are deleted from code, since not needed

manageAdmin func and this var are deleted from code, since not needed

manageAdmin func and this var are deleted from code, since not needed
}
// Insert the current user as member and admin
tm := TeamMember{TeamID: t.ID, Username: doer.Username, Admin: false}
if err = tm.Create(s, doer); err != nil {
return err
}
return events.Dispatch(&TeamCreatedEvent{
Team: t,
Doer: a,
})
viehlieb marked this conversation as resolved Outdated

That's not what the function seems to be doing. Please change the name or comment.

That's not what the function seems to be doing. Please change the name or comment.
}
// Delete deletes a team
// @Summary Deletes a team
// @Description Delets a team. This will also remove the access for all users in that team.

View File

@ -53,16 +53,18 @@ type Provider struct {
AuthURL string `json:"auth_url"`
LogoutURL string `json:"logout_url"`
ClientID string `json:"client_id"`
Scope string `json:"scope"`
ClientSecret string `json:"-"`
openIDProvider *oidc.Provider
Oauth2Config *oauth2.Config `json:"-"`
}
type claims struct {
viehlieb marked this conversation as resolved Outdated

Didn't you declare this already somewhere else?

Didn't you declare this already somewhere else?
Email string `json:"email"`
Name string `json:"name"`
PreferredUsername string `json:"preferred_username"`
Nickname string `json:"nickname"`
Email string `json:"email"`
Name string `json:"name"`
PreferredUsername string `json:"preferred_username"`
Nickname string `json:"nickname"`
Group []string `json:"groups"`
}
func init() {
@ -192,22 +194,79 @@ func HandleCallback(c echo.Context) error {
// Check if we have seen this user before
u, err := getOrCreateUser(s, cl, idToken.Issuer, idToken.Subject)
log.Errorf("Issuer %s: %v", idToken.Issuer, err)
if err != nil {
_ = s.Rollback()
viehlieb marked this conversation as resolved Outdated

Please remove this line.

Please remove this line.
log.Errorf("Error creating new user for provider %s: %v", provider.Name, err)
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.
return handler.HandleHTTPError(err, c)
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.
}
// Check if we have seen this user before
teams, err := GetOrCreateTeamsByNames(s, cl.Group, u)
if err != nil {
log.Errorf("Error verifying team for name %v, got %v", cl.Name, teams, err)
return 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.
} else {
for _, team := range teams {
tm := models.TeamMember{TeamID: team.ID, Username: u.Username}
if err = tm.Create(s, u); err != nil {
switch t := err.(type) {
case *models.ErrUserIsMemberOfTeam:
viehlieb marked this conversation as resolved Outdated

Please don't ignore the error.

Please don't ignore the error.
log.Errorf("ErrUserIsMemberOfTeam", t)
break
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.
default:
log.Errorf("Error assigning User to team", t)
}
}
}
}
konrad marked this conversation as resolved Outdated

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.

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 {
return handler.HandleHTTPError(err, c)
}
// Create token
return auth.NewUserAuthTokenResponse(u, c, false)
}
viehlieb marked this conversation as resolved Outdated

Replace with

if len(teamData) < 1 {
	return
}
Replace with ``` if len(teamData) < 1 { return }

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.

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
}
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 } ```

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

This should be an Error log message.

This should be an `Error` log message.
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 models.IsErrTeamsDoNotExist(err) {
log.Errorf("No such Team: %v, got %v", t, team, err)
tea := &models.Team{
Name: t,
}
err := tea.CreateNoAdmin(s, u)
if err != nil {
log.Errorf("Teams: %v, err: %v", tea, err)
} else {
te = append(te, *tea)
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.
}
} else {
// if multiple teams with same name are found,
if len(team) == 1 {
konrad marked this conversation as resolved Outdated

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.

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.

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`.

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.

> 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.

okay, teams won't be deleted

okay, teams won't be deleted
te = append(te, *team[len(team)-1])
} else {
log.Errorf("Multiple Teams have the same name: %v, ", team[len(team)-1].Name)
viehlieb marked this conversation as resolved Outdated

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.
}
}
}
viehlieb marked this conversation as resolved Outdated

why not do this in one sql query?

why not do this in one sql query?

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 ```

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 ```

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{})`

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 ```
return te, err
viehlieb marked this conversation as resolved Outdated

Please return the error instead of logging only.

Please return the error instead of logging only.

Please don't ignore the errors.

Please don't ignore the errors.

not ignored now

not ignored now
}
// assign user to team
// remove user from team if not in group
// if multiple teams found with same name -> do nothing
// optional: assign by id
//
viehlieb marked this conversation as resolved Outdated

This whole inner loop should be rewritten to something like this so that it's more idomatic 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
}
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 }

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.

changed behaviour to not delete team

changed behaviour to not delete team
func getOrCreateUser(s *xorm.Session, cl *claims, issuer, subject string) (u *user.User, err error) {
// Check if the user exists for that issuer and subject
viehlieb marked this conversation as resolved Outdated

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.

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?

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?

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.

> 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.

edited

edited

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

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

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?

> 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?

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.

> 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.

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.

> 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.
u, err = user.GetUserWithEmail(s, &user.User{
Issuer: issuer,