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 89 additions and 1 deletions
Showing only changes of commit ed91d6744e - Show all commits

View File

@ -1081,7 +1081,33 @@ func (err ErrTeamDoesNotExist) HTTPError() web.HTTPError {
return web.HTTPError{HTTPCode: http.StatusNotFound, Code: ErrCodeTeamDoesNotExist, Message: "This team does not exist."}
}
<<<<<<< HEAD
// ErrTeamAlreadyHasAccess represents an error where a team already has access to a 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)
}
// ErrCodeTeamDoesNotExist holds the unique world-error code of this error
const ErrCodeTeamsDoNotExist = 6002
// 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."}
}
// ErrTeamAlreadyHasAccess represents an error where a team already has access to a list/namespace
>>>>>>> 2715a556... introduce functionality to assign/create team via group claim
type ErrTeamAlreadyHasAccess struct {
TeamID int64
ID int64

View File

@ -81,7 +81,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.
}
@ -128,6 +128,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}
}
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
var ts []*Team
exists := s.
Where("name = ?", name).
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
Find(&ts)
if exists != nil {
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`.
return
}
if len(ts) == 0 {
return ts, ErrTeamsDoNotExist{name}
}
// //for each ts
// teamSlice := []*Team{ts}
// err = addMoreInfoToTeams(s, teamSlice)
// if err != nil {
// return
// }
viehlieb marked this conversation as resolved Outdated

Please handle the error.

Please handle the error.
teams = ts
return
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.
}
// GetTeamByOidcIDAndName gets teams where oidc_id and name match parameters
// For oidc team creation oidcID and Name need to be set
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.
@ -319,6 +347,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
}
// 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,
})
}
// 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

@ -195,6 +195,9 @@ 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 {
viehlieb marked this conversation as resolved Outdated

Please remove this line.

Please remove this line.
_ = s.Rollback()
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.
log.Errorf("Error creating new user for provider %s: %v", provider.Name, err)
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.