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
3 changed files with 117 additions and 9 deletions
Showing only changes of commit a96dc36d58 - Show all commits

View File

@ -1059,7 +1059,6 @@ func (err ErrTeamNameCannotBeEmpty) HTTPError() web.HTTPError {
return web.HTTPError{HTTPCode: http.StatusBadRequest, Code: ErrCodeTeamNameCannotBeEmpty, Message: "The team name cannot be empty"}
}
// ErrTeamDoesNotExist represents an error where a team does not exist
type ErrTeamDoesNotExist struct {
TeamID int64
}
@ -1178,6 +1177,51 @@ func (err ErrTeamDoesNotHaveAccessToProject) HTTPError() web.HTTPError {
return web.HTTPError{HTTPCode: http.StatusForbidden, Code: ErrCodeTeamDoesNotHaveAccessToProject, Message: "This team does not have access to the project."}
}
type ErrTeamsDoNotExist struct {
Name string
}
// IsErrTeamDoNotExist checks if an error is ErrTeamDoesNotExist.
func IsErrTeamsDoNotExist(err error) bool {
_, ok := err.(ErrTeamsDoNotExist)
return ok
}
func (err ErrTeamsDoNotExist) Error() string {
return fmt.Sprintf("Team does not exist [Team Name: %v]", err.Name)
}
viehlieb marked this conversation as resolved Outdated

Please add a comment, similar to the other errors.

Please add a comment, similar to the other errors.
// ErrCodeTeamDoesNotExist holds the unique world-error code of this error
const ErrCodeTeamsDoNotExist = 6008
// HTTPError holds the http error description
func (err ErrTeamsDoNotExist) HTTPError() web.HTTPError {
return web.HTTPError{HTTPCode: http.StatusNotFound, Code: ErrCodeTeamDoesNotExist, Message: "No team with given name exists."}
}
// ErrOIDCTeamsDoNotExistForUser represents an error where an oidcTeam does not exist for the user
type ErrOIDCTeamsDoNotExistForUser struct {
UserID int64
viehlieb marked this conversation as resolved Outdated

Is this an error about one team not existing or multiple? Because the error name suggests one or more teams but the error message is about only one.

Is this an error about one team not existing or multiple? Because the error name suggests one or more teams but the error message is about only one.
}
// IsErrOIDCTeamsDoNotExistForUser checks if an error is ErrOIDCTeamsDoNotExistForUser.
func IsErrOIDCTeamsDoNotExistForUser(err error) bool {
viehlieb marked this conversation as resolved Outdated

Please add the error to the error docs.

Please add the error to the error docs.
_, ok := err.(ErrTeamDoesNotExist)
return ok
}
viehlieb marked this conversation as resolved Outdated

Please change the message to "No team with that name exists"

Please change the message to "No team with that name exists"

done all the error stuff

done all the error stuff
func (err ErrOIDCTeamsDoNotExistForUser) Error() string {
return fmt.Sprintf("No Oidc exists for User [User ID: %d]", err.UserID)
}
// ErrCodeTeamDoesNotExist holds the unique world-error code of this error
const ErrCodeOIDCTeamsDoNotExistForUser = 6009
// HTTPError holds the http error description
func (err ErrOIDCTeamsDoNotExistForUser) HTTPError() web.HTTPError {
return web.HTTPError{HTTPCode: http.StatusNotFound, Code: ErrCodeTeamDoesNotExist, Message: "This team does not exist."}
}
// ====================
// User <-> Project errors
// ====================
viehlieb marked this conversation as resolved Outdated

What OIDC does not exist? What does that mean?

What OIDC does not exist? What does that mean?

View File

@ -94,6 +94,12 @@ type TeamUser struct {
TeamID int64 `json:"-"`
}
type TeamData struct {
viehlieb marked this conversation as resolved Outdated

What is this used for? Please add a comment.

What is this used for? Please add a comment.

What's the difference to the Team struct?

What's the difference to the `Team` struct?

Intermediate struct which only holds TeamName and OidcId Description.
It does not exist as a Team yet or better: it is the data accessible via oidc, which the Team struct is compared against.

Intermediate struct which only holds TeamName and OidcId Description. It does not exist as a Team yet or better: it is the data accessible via oidc, which the Team struct is compared against.

Okay, but isn't it missing the json tags then? I'm not sure if I understood you correctly herre.

If it's only used for oidc data the name should reflect that. TeamData is too generic.

Okay, but isn't it missing the json tags then? I'm not sure if I understood you correctly herre. If it's only used for oidc data the name should reflect that. `TeamData` is too generic.

No, the teamData has to be pulled out of token via

getTeamDataFromToken

Called it OIDCTeamData now.

No, the teamData has to be pulled out of token via `getTeamDataFromToken` Called it OIDCTeamData now.
TeamName string
OidcID string
Description string
}
// GetTeamByID gets a team by its ID
func GetTeamByID(s *xorm.Session, id int64) (team *Team, err error) {
if id < 1 {
@ -143,16 +149,30 @@ func GetTeamsByName(s *xorm.Session, name string) (teams []*Team, err error) {
// GetTeamByOidcIDAndName gets teams where oidc_id and name match parameters
// For oidc team creation oidcID and Name need to be set
func GetTeamByOidcIDAndName(s *xorm.Session, id string, name string) (team Team, err error) {
func GetTeamByOidcIDAndName(s *xorm.Session, oidcID string, teamName string) (team Team, err error) {
exists, err := s.
Table("teams").
viehlieb marked this conversation as resolved Outdated

Please handle the error.

Please handle the error.
Where("oidc_id = ? AND name = ?", id, name).
Where("oidc_id = ? AND name = ?", oidcID, teamName).
Get(&team)
log.Debugf("GetTeamByOidcIDAndName: %v, exists: %v", team.Name, exists)
viehlieb marked this conversation as resolved Outdated

What happens if there's more than one team with that combination (unlikely, but not handled)

What happens if there's more than one team with that combination (unlikely, but not handled)

I tend to just changing functionality to take the first team with that specific setting.

I tend to just changing functionality to take the first team with that specific setting.

That sounds like it could work. Easiest way would be to add a OrderBy clause here.

That sounds like it could work. Easiest way would be to add a `OrderBy` clause here.
if exists && err == nil {
return team, nil
}
return team, ErrTeamsDoNotExist{id}
return team, ErrTeamsDoNotExist{oidcID}
viehlieb marked this conversation as resolved Outdated

This error message should contain the name as well.

This error message should contain the name as well.
}
func FindAllOidcTeamIDsForUser(s *xorm.Session, userID int64) (ts []int64, err error) {
err = s.
Table("team_members").
Where("user_id = ? ", userID).
Join("RIGHT", "teams", "teams.id = team_members.team_id").
Where("teams.oidc_id != ?", "").
viehlieb marked this conversation as resolved Outdated

Can teams.oidc_id be null? Then you should check that as well.

Can `teams.oidc_id` be null? Then you should check that as well.
Cols("teams.id").
Find(&ts)
if ts == nil || err != nil {
return ts, ErrOIDCTeamsDoNotExistForUser{userID}
}
return ts, nil
viehlieb marked this conversation as resolved Outdated

Why do you only load the ids here?

Why do you only load the ids here?

just ids are needed here to prevent overhead.

just ids are needed here to prevent overhead.
}
func addMoreInfoToTeams(s *xorm.Session, teams []*Team) (err error) {

View File

@ -213,16 +213,22 @@ func HandleCallback(c echo.Context) error {
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.
// find or create Teams and assign user as teammember.
viehlieb marked this conversation as resolved Outdated

Please don't ignore the error.

Please don't ignore the error.
//TODO: fix this error check
// nil is no problem
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.
if len(teamData) > 0 {
//find old teams for user through oidc
oldOidcTeams, _ := models.FindAllOidcTeamIDsForUser(s, u.ID)
// check if we have seen these teams before.
// find or create Teams and assign user as teammember.
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..
var oidcTeams []int64
log.Debugf("TeamData is set %v", teamData)
teams, err := GetOrCreateTeamsByOIDCAndNames(s, teamData, u)
if err != nil {
log.Errorf("Error verifying team for name %v, got %v", cl.Name, teams, err)
return err
}
for _, team := range teams {
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.
tm := models.TeamMember{TeamID: team.ID, Username: u.Username}
viehlieb marked this conversation as resolved Outdated

This should be an Error log message.

This should be an `Error` log message.
exists, err := tm.CheckMembership(s)
@ -234,9 +240,10 @@ func HandleCallback(c echo.Context) error {
} else {
log.Debugf("Team exists? %v or error: %v", exists, err)
}
oidcTeams = append(oidcTeams, team.ID)
}
SignOutFromOrDeleteTeamsByID(s, u, notIn(oldOidcTeams, oidcTeams))
}
err = s.Commit()
if err != nil {
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.
_ = s.Rollback()
@ -247,13 +254,30 @@ func HandleCallback(c echo.Context) error {
return auth.NewUserAuthTokenResponse(u, c, false)
}
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.
func SignOutFromOrDeleteTeamsByID(s *xorm.Session, u *user.User, teamIDs []int64) {
for _, teamID := range teamIDs {
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 ```
tm := models.TeamMember{TeamID: teamID, Username: u.Username}
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
err := tm.Delete(s, u)
if err != nil {
team, err := models.GetTeamByID(s, teamID)
if err != nil {
log.Errorf("Cannot find team with id: %v, err: %v", teamID, err)
} else {
err = team.Delete(s, u)
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
if err != nil {
log.Errorf("Cannot delete team %v", err)
}
}
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.
}
}
}
viehlieb marked this conversation as resolved Outdated

Why not cast this to []map[string]interface{} directly?

Why not cast this to `[]map[string]interface{}` directly?

no reason,
had claim VikunjaGroups more agnostic to specific data type.
now casts directly to []map[string]interface{}

no reason, had claim VikunjaGroups more agnostic to specific data type. now casts directly to []map[string]interface{}
func getTeamDataFromToken(groups interface{}, provider *Provider) (teamData []TeamData, err error) {
teamData = []TeamData{}
if groups != nil {
el := groups.([]interface{})
for _, data := range el {
team := data.(map[string]interface{})
log.Debugf("%s", team)
var name string
var description string
var oidcID string
@ -394,3 +418,23 @@ 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 {
viehlieb marked this conversation as resolved Outdated

Don't you mean int?

Don't you mean int?
if s1 == s2 {
found = true
break
}
}
// String not found. We add it to return slice
if !found {
diff = append(diff, s1)
}
}
return diff
}