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
Contributor

Resolves #1279
Resolves https://github.com/go-vikunja/vikunja/issues/42
Resolves #950

frontend adaptations:
vikunja/frontend#3033

assigning teams to users via oidc.

Please refer to config.yml.sample to set it up and openid.md for a detailed explanation on how to the feature works.

happy to get feedback.

cheers.

Resolves https://kolaente.dev/vikunja/vikunja/issues/1279 Resolves https://github.com/go-vikunja/vikunja/issues/42 Resolves https://kolaente.dev/vikunja/vikunja/issues/950 frontend adaptations: https://kolaente.dev/vikunja/frontend/pulls/3033 assigning teams to users via oidc. Please refer to config.yml.sample to set it up and openid.md for a detailed explanation on how to the feature works. happy to get feedback. cheers.
viehlieb added 18 commits 2023-02-10 21:30:09 +00:00
konrad requested changes 2023-02-13 10:13:29 +00:00
konrad left a comment
Owner

Please fix the lint and regenerate config docs.

Please fix the lint and regenerate config docs.
konrad requested changes 2023-02-13 10:43:15 +00:00
@ -1194,0 +1201,4 @@
}
func (err ErrTeamsDoNotExist) Error() string {
return fmt.Sprintf("Team does not exist [Team Name: %v]", err.Name)
Owner

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.
viehlieb marked this conversation as resolved
@ -1194,0 +1205,4 @@
}
// ErrCodeTeamDoesNotExist holds the unique world-error code of this error
const ErrCodeTeamsDoNotExist = 6008
Owner

Please add the error to the error docs.

Please add the error to the error docs.
viehlieb marked this conversation as resolved
@ -1194,0 +1209,4 @@
// 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."}
Owner

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

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

done all the error stuff

done all the error stuff
viehlieb marked this conversation as resolved
@ -1194,0 +1224,4 @@
}
func (err ErrOIDCTeamsDoNotExistForUser) Error() string {
return fmt.Sprintf("No Oidc exists for User [User ID: %d]", err.UserID)
Owner

What OIDC does not exist? What does that mean?

What OIDC does not exist? What does that mean?
viehlieb marked this conversation as resolved
@ -110,2 +110,4 @@
}
func (tm *TeamMember) CheckMembership(s *xorm.Session) (exists bool, err error) {
member, err := user2.GetUserByUsername(s, tm.Username)
Owner

Can you do this either with a join to avoid loading the user or at least use the "simple" function?

Can you do this either with a join to avoid loading the user or at least use the "simple" function?
Author
Contributor

done.

done.
viehlieb marked this conversation as resolved
@ -80,3 +83,3 @@
// TableName makes beautiful table names
func (*TeamMember) TableName() string {
func (TeamMember) TableName() string {
Owner

Why did you change this?

Why did you change this?
Author
Contributor

oha.
this was unintended.

oha. this was unintended.
viehlieb marked this conversation as resolved
@ -91,6 +94,12 @@ type TeamUser struct {
TeamID int64 `json:"-"`
}
type TeamData struct {
Owner

What is this used for? Please add a comment.

What is this used for? Please add a comment.
viehlieb marked this conversation as resolved
@ -123,0 +158,4 @@
if exists && err == nil {
return team, nil
}
return team, ErrTeamsDoNotExist{oidcID}
Owner

This error message should contain the name as well.

This error message should contain the name as well.
viehlieb marked this conversation as resolved
@ -123,0 +166,4 @@
Table("team_members").
Where("user_id = ? ", userID).
Join("RIGHT", "teams", "teams.id = team_members.team_id").
Where("teams.oidc_id != ?", "").
Owner

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.
viehlieb marked this conversation as resolved
@ -123,0 +172,4 @@
if ts == nil || err != nil {
return ts, ErrOIDCTeamsDoNotExistForUser{userID}
}
return ts, nil
Owner

Why do you only load the ids here?

Why do you only load the ids here?
Author
Contributor

just ids are needed here to prevent overhead.

just ids are needed here to prevent overhead.
viehlieb marked this conversation as resolved
@ -273,2 +328,2 @@
// Insert the current user as member and admin
tm := TeamMember{TeamID: t.ID, Username: doer.Username, Admin: true}
var admin = true
tm := TeamMember{TeamID: t.ID, Username: doer.Username, Admin: admin}
Owner

Why the extra variable?

Why the extra variable?
Author
Contributor

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

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

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

manageAdmin func and this var are deleted from code, since not needed
viehlieb marked this conversation as resolved
@ -283,2 +338,4 @@
}
func (t *Team) ManageAdminRight(teamMember TeamMember, admin bool) {
// Insert the current user as member and admin
Owner

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.
viehlieb marked this conversation as resolved
@ -58,3 +60,3 @@
Oauth2Config *oauth2.Config `json:"-"`
}
type TeamData struct {
Owner

Didn't you declare this already somewhere else?

Didn't you declare this already somewhere else?
viehlieb marked this conversation as resolved
@ -66,0 +69,4 @@
Name string `json:"name"`
PreferredUsername string `json:"preferred_username"`
Nickname string `json:"nickname"`
Teams []string `json:"groups"`
Owner

Please rename this to `Groups

Please rename this to `Groups
Owner

In the documentation you said only vikunja_groups are used. Does that mean this attribute is not needed at all?

In the documentation you said only `vikunja_groups` are used. Does that mean this attribute is not needed at all?
Author
Contributor

as a matter of fact, that's true.
removed

as a matter of fact, that's true. *removed*
viehlieb marked this conversation as resolved
@ -66,0 +70,4 @@
PreferredUsername string `json:"preferred_username"`
Nickname string `json:"nickname"`
Teams []string `json:"groups"`
VikunjaGroups interface{} `json:"vikunja_groups"`
Owner

What is this used for? (While having `groups

What is this used for? (While having `groups
viehlieb marked this conversation as resolved
@ -189,22 +198,150 @@ func HandleCallback(c echo.Context) error {
// Check if we have seen this user before
u, err := getOrCreateUser(s, cl, idToken.Issuer, idToken.Subject)
Owner

Please remove this line.

Please remove this line.
viehlieb marked this conversation as resolved
@ -201,2 +213,4 @@
}
//find old teams for user through oidc
oldOidcTeams, _ := models.FindAllOidcTeamIDsForUser(s, u.ID)
Owner

Please don't ignore the error.

Please don't ignore the error.
viehlieb marked this conversation as resolved
@ -203,0 +230,4 @@
if !exists {
err = tm.Create(s, u)
if err != nil {
log.Debugf("Could not assign %v to %v. %v", u.Username, team.Name, err)
Owner

This should be an Error log message.

This should be an `Error` log message.
viehlieb marked this conversation as resolved
@ -204,3 +249,4 @@
return auth.NewUserAuthTokenResponse(u, c, false)
}
func SignOutFromOrDeleteTeamsByID(s *xorm.Session, u *user.User, teamIDs []int64) {
Owner

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

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

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

okay, teams won't be deleted

okay, teams won't be deleted
konrad marked this conversation as resolved
@ -207,0 +256,4 @@
if err != nil {
team, err := models.GetTeamByID(s, teamID)
if err != nil {
log.Errorf("Cannot find team with id: %v, err: %v", teamID, err)
Owner

Please return the error instead of logging only.

Please return the error instead of logging only.
viehlieb marked this conversation as resolved
@ -207,0 +263,4 @@
log.Errorf("Cannot delete team %v", err)
}
}
}
Owner

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

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

changed behaviour to not delete team

changed behaviour to not delete team
viehlieb marked this conversation as resolved
@ -207,0 +270,4 @@
func getTeamDataFromToken(groups interface{}, provider *Provider) (teamData []TeamData, err error) {
teamData = []TeamData{}
if groups != nil {
el := groups.([]interface{})
Owner

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

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

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{}
viehlieb marked this conversation as resolved
@ -207,0 +285,4 @@
if team["oidcID"] != nil {
switch t := team["oidcID"].(type) {
case int64:
oidcID = fmt.Sprintf("%v", team["oidcID"])
Owner

Please use `strconv.FormatInt

Please use `strconv.FormatInt
Author
Contributor

done

done
viehlieb marked this conversation as resolved
@ -207,0 +290,4 @@
oidcID = string(team["oidcID"].(string))
case float64:
fl := float64(team["oidcID"].(float64))
oidcID = fmt.Sprintf("%v", fl)
Owner

Please use strconv.FormatFloat (not 100% sure if the function is really called that)

Please use `strconv.FormatFloat` (not 100% sure if the function is really called that)
viehlieb marked this conversation as resolved
@ -207,0 +297,4 @@
}
if name == "" || oidcID == "" {
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{}
Owner

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

changed behaviour to skipping the invalid team

changed behaviour to skipping the invalid team
viehlieb marked this conversation as resolved
@ -207,0 +311,4 @@
Description: teamData.Description,
OidcID: teamData.OidcID,
}
log.Debugf("Teams: %v", tea.OidcID)
Owner

Please make this debug message more descriptive.

Please make this debug message more descriptive.
viehlieb marked this conversation as resolved
@ -207,0 +319,4 @@
// 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) {
te = []models.Team{}
Owner

Please use

Please use
Author
Contributor

Here there is the information missing, what do you suggest?

Here there is the information missing, what do you suggest?
Owner

Sorry, I meant []*models.Team{}.

Sorry, I meant `[]*models.Team{}`.
viehlieb marked this conversation as resolved
@ -207,0 +336,4 @@
}
}
log.Debugf("Array: %v", te)
Owner

Please make this debug message more descriptive or remove it alltogether.

Please make this debug message more descriptive or remove it alltogether.
viehlieb marked this conversation as resolved
@ -277,1 +414,4 @@
}
// find the elements which appear in slice1,but not in slice2
func notIn(slice1 []int64, slice2 []int64) []int64 {
Owner

Can you move this to the utils package? Is there really nothing like this in the standard library?

Can you move this to the `utils` package? Is there really nothing like this in the standard library?
viehlieb marked this conversation as resolved
@ -278,0 +425,4 @@
break
}
}
// String not found. We add it to return slice
Owner

Don't you mean int?

Don't you mean int?
viehlieb marked this conversation as resolved
@ -0,0 +1,99 @@
regarding:
Owner

Is this document final?

Is this document final?
viehlieb marked this conversation as resolved
@ -0,0 +5,4 @@
This PR adds the functionality to assign users to teams via oidc.
Read carefully and brief your administrators to use this feature.
Tested with oidc provider authentik.
To distinguish between groups created in vikunja and groups generated via oidc, there is an attribute neccessary, which is called: *oidcID*
Owner

Where is that attribute placed? In the provider or Vikunja? How is it relevant for admins?

Where is that attribute placed? In the provider or Vikunja? How is it relevant for admins?
Author
Contributor

At that location, it is just an information as to what is happening at all.

At that location, it is just an information as to what is happening at all.
Owner

Okay, we should just think of fixing it before merging the PR

Okay, we should just think of fixing it before merging the PR
Author
Contributor

What's your suggested change here?
The Data from oidc needs to be in the form, that is described in openid.md
Therefore it is relevant for adminis, that operate authentik, keycloak, etc..

What's your suggested change here? The Data from oidc needs to be in the form, that is described in openid.md Therefore it is relevant for adminis, that operate authentik, keycloak, etc..
Owner

Okay, so Admins from the external provider need to configure their provider to send an oidcID to Vikunja? Because that's what I'm interpreting this sentence as but that's not what it says.

Okay, so Admins from the external provider need to configure their provider to send an `oidcID` to Vikunja? Because that's what I'm interpreting this sentence as but that's not what it says.
viehlieb marked this conversation as resolved
@ -0,0 +8,4 @@
To distinguish between groups created in vikunja and groups generated via oidc, there is an attribute neccessary, which is called: *oidcID*
## Setup
Edit config.yml to include scope: openid profile email vikunja_scope
Owner

Please link to the config option in the config doc.

Please link to the config option in the config doc.
viehlieb marked this conversation as resolved
@ -0,0 +50,4 @@
- You will see "(sso: XXXXX)" written next to each team you were asigned through oidc.
## IMPORTANT NOTES:
Owner

Please make this a list.

Please make this a list.
viehlieb marked this conversation as resolved
konrad changed title from 950_reworked_assign_teams_via_oidc to feat: assign users to teams via OIDC claims 2023-02-13 10:43:49 +00:00
viehlieb added 7 commits 2023-02-13 19:04:38 +00:00
viehlieb added 1 commit 2023-02-14 10:08:28 +00:00
fix lint
Some checks failed
continuous-integration/drone/pr Build is failing
82b7c14fed
konrad reviewed 2023-02-14 11:40:57 +00:00
@ -113,1 +113,4 @@
| 6007 | 403 | The team does not have access to the list to perform that action. |
| 6008 | 400 | There are no teams found with that team name |
| 6009 | 400 | There is no oidc team with that team name and oidcId |
| 6010 | 400 | There are no oidc teams found for the user |
Owner

Please add punctuation.

Please add punctuation.
viehlieb marked this conversation as resolved
@ -1073,4 +1073,3 @@
}
// ErrTeamDoesNotExist represents an error where a team does not exist
type ErrTeamDoesNotExist struct {
Owner

Please don't remove the comment.

Please don't remove the comment.
viehlieb marked this conversation as resolved
@ -1191,6 +1190,75 @@ func (err ErrTeamDoesNotHaveAccessToList) HTTPError() web.HTTPError {
return web.HTTPError{HTTPCode: http.StatusForbidden, Code: ErrCodeTeamDoesNotHaveAccessToList, Message: "This team does not have access to the list."}
}
type ErrTeamsDoNotExist struct {
Owner

Please add a comment, similar to the other errors.

Please add a comment, similar to the other errors.
viehlieb marked this conversation as resolved
@ -112,0 +111,4 @@
func (tm *TeamMember) CheckMembership(s *xorm.Session) (exists bool, err error) {
exists, err = s.
Where("team_id = ? AND user_id = ?", tm.TeamID, tm.UserID).
Get(&TeamMember{})
Owner

This should be Exist(&TeamMember{}).

This should be `Exist(&TeamMember{})`.
Author
Contributor

done

done
viehlieb marked this conversation as resolved
@ -112,0 +120,4 @@
err = s.
Where("team_id = ?", tm.TeamID).
Cols("user_id").
Find(&members)
Owner

If it's only counting, please use Count().

If it's only counting, please use `Count()`.
Author
Contributor

done

done
viehlieb marked this conversation as resolved
@ -91,6 +94,13 @@ type TeamUser struct {
TeamID int64 `json:"-"`
}
// TeamData is the relevant data for a team and is delivered by oidc token
Owner

What's the difference to the Team struct?

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

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

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

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.
viehlieb marked this conversation as resolved
@ -207,0 +256,4 @@
for _, teamID := range teamIDs {
tm := models.TeamMember{TeamID: teamID, UserID: u.ID, Username: u.Username}
exists, _ := tm.CheckMembership(s)
memberCount, _ := tm.GetMemberCount(s)
Owner

Please don't ignore the errors.

Please don't ignore the errors.
Author
Contributor

not ignored now

not ignored now
viehlieb marked this conversation as resolved
viehlieb force-pushed 950_reworked_assign_teams_via_oidc from af2cd7a42e to d3ca751f4c 2023-02-23 14:42:30 +00:00 Compare
Author
Contributor

Please fix the lint and regenerate config docs.

I try to fix lint and it tells me to run 'do-the-swag'. which i do, it runs the commands, etc... but no new swagger file is created.
Any idea?

> Please fix the lint and regenerate config docs. I try to fix lint and it tells me to run 'do-the-swag'. which i do, it runs the commands, etc... but no new swagger file is created. Any idea?
viehlieb added 2 commits 2023-02-23 14:52:28 +00:00
viehlieb force-pushed 950_reworked_assign_teams_via_oidc from 5bd7451469 to fc89564ebc 2023-02-23 15:27:46 +00:00 Compare
viehlieb added 1 commit 2023-02-23 15:33:36 +00:00
change too generic name TeamData struct to OIDCTeamData
Some checks failed
continuous-integration/drone/pr Build is failing
b607fea49d
konrad requested changes 2023-02-23 15:34:32 +00:00
@ -112,0 +117,4 @@
func (tm *TeamMember) GetMemberCount(s *xorm.Session) (memberCount int64, err error) {
member := TeamMember{}
memberCount, err = s.
Owner

Couldn't you just return the result here directly?

Couldn't you just return the result here directly?
Author
Contributor

In fact this function is not needed anymore for this feature, so I'll just remove it.
It was used for finding out whether a user should be signed out from team or deleted.

In fact this function is not needed anymore for this feature, so I'll just remove it. It was used for finding out whether a user should be signed out from team or deleted.
viehlieb marked this conversation as resolved
@ -123,0 +136,4 @@
return teams, ErrTeamsDoNotExist{name}
}
var ts []*Team
Owner

Did you try passing the

Did you try passing the
Author
Contributor

Ups, I guess here is something missing

Ups, I guess here is something missing
konrad marked this conversation as resolved
@ -123,0 +151,4 @@
// 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, oidcID string, teamName string) (team Team, err error) {
exists, err := s.
Owner

Please handle the error.

Please handle the error.
viehlieb marked this conversation as resolved
@ -123,0 +154,4 @@
exists, err := s.
Table("teams").
Where("oidc_id = ? AND name = ?", oidcID, teamName).
Get(&team)
Owner

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)
Author
Contributor

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

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.
viehlieb marked this conversation as resolved
@ -197,1 +200,4 @@
// does the oidc token contain well formed "vikunja_groups" through vikunja_scope
teamData, errs := getTeamDataFromToken(cl.VikunjaGroups, provider)
for _, err := range errs {
Owner

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

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

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.
viehlieb marked this conversation as resolved
@ -207,0 +252,4 @@
return oidcTeams, err
}
func SignOutFromTeamsByID(s *xorm.Session, u *user.User, teamIDs []int64) (errs []error) {
Owner

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
@ -207,0 +267,4 @@
err = tm.Delete(s, u)
// if you cannot delete the team_member
if err != nil {
errs = append(errs, err)
Owner

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

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

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

edited

edited
Author
Contributor

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
Owner

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

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

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
Owner

Please fix the lint and regenerate config docs.

I try to fix lint and it tells me to run 'do-the-swag'. which i do, it runs the commands, etc... but no new swagger file is created.
Any idea?

When I run it locally it seems to re-generate everything as it should.

There's an error in the CI:

2023/02/23 15:14:05 warning: failed to evaluate const locb at /go/pkg/mod/github.com/pelletier/go-toml/v2@v2.0.6/internal/characters/utf8.go:183:2, strconv.ParseUint: parsing "b10000000": invalid syntax

2023/02/23 15:14:05 warning: failed to evaluate const hicb at /go/pkg/mod/github.com/pelletier/go-toml/v2@v2.0.6/internal/characters/utf8.go:184:2, strconv.ParseUint: parsing "b10111111": invalid syntax

Try removing the swaggo binary (rm ~/go/bin/swag) and then re-run the command, at least then you should get the same error message.

> > Please fix the lint and regenerate config docs. > > I try to fix lint and it tells me to run 'do-the-swag'. which i do, it runs the commands, etc... but no new swagger file is created. > Any idea? When I run it locally it seems to re-generate everything as it should. There's an error in the CI: ``` 2023/02/23 15:14:05 warning: failed to evaluate const locb at /go/pkg/mod/github.com/pelletier/go-toml/v2@v2.0.6/internal/characters/utf8.go:183:2, strconv.ParseUint: parsing "b10000000": invalid syntax 2023/02/23 15:14:05 warning: failed to evaluate const hicb at /go/pkg/mod/github.com/pelletier/go-toml/v2@v2.0.6/internal/characters/utf8.go:184:2, strconv.ParseUint: parsing "b10111111": invalid syntax ``` Try removing the swaggo binary (`rm ~/go/bin/swag`) and then re-run the command, at least then you should get the same error message.
viehlieb added 1 commit 2023-03-10 12:46:38 +00:00
remove left over function GetMemberCount, rename function SignOut to RemoveFrom
Some checks failed
continuous-integration/drone/pr Build is failing
169b668c3c
Author
Contributor

Please fix the lint and regenerate config docs.

I try to fix lint and it tells me to run 'do-the-swag'. which i do, it runs the commands, etc... but no new swagger file is created.
Any idea?

When I run it locally it seems to re-generate everything as it should.

There's an error in the CI:

2023/02/23 15:14:05 warning: failed to evaluate const locb at /go/pkg/mod/github.com/pelletier/go-toml/v2@v2.0.6/internal/characters/utf8.go:183:2, strconv.ParseUint: parsing "b10000000": invalid syntax

2023/02/23 15:14:05 warning: failed to evaluate const hicb at /go/pkg/mod/github.com/pelletier/go-toml/v2@v2.0.6/internal/characters/utf8.go:184:2, strconv.ParseUint: parsing "b10111111": invalid syntax

Try removing the swaggo binary (rm ~/go/bin/swag) and then re-run the command, at least then you should get the same error message.

great, will do that after the next step of change. Thanks for the advice.

> > > Please fix the lint and regenerate config docs. > > > > I try to fix lint and it tells me to run 'do-the-swag'. which i do, it runs the commands, etc... but no new swagger file is created. > > Any idea? > > When I run it locally it seems to re-generate everything as it should. > > There's an error in the CI: > > ``` > 2023/02/23 15:14:05 warning: failed to evaluate const locb at /go/pkg/mod/github.com/pelletier/go-toml/v2@v2.0.6/internal/characters/utf8.go:183:2, strconv.ParseUint: parsing "b10000000": invalid syntax > > 2023/02/23 15:14:05 warning: failed to evaluate const hicb at /go/pkg/mod/github.com/pelletier/go-toml/v2@v2.0.6/internal/characters/utf8.go:184:2, strconv.ParseUint: parsing "b10111111": invalid syntax > ``` > > Try removing the swaggo binary (`rm ~/go/bin/swag`) and then re-run the command, at least then you should get the same error message. great, will do that after the next step of change. Thanks for the advice.
viehlieb added 2 commits 2023-03-30 15:15:52 +00:00
return single team for GetTeamByOidcIDAndName
Some checks failed
continuous-integration/drone/pr Build is failing
146fa2c70a
Author
Contributor

hey @konrad . Did you have the chance to look into the changes?
main branch development is continuing, which makes it harder to keep up on track.

hey @konrad . Did you have the chance to look into the changes? main branch development is continuing, which makes it harder to keep up on track.
Owner

Can you check the unresolved discussions? Please comment and resolve all discussions which are done now.

Can you check the unresolved discussions? Please comment and resolve all discussions which are done now.
Author
Contributor

Can you check the unresolved discussions? Please comment and resolve all discussions which are done now.

Hi. There is one discussion still open and I commented on it. could you check?

> Can you check the unresolved discussions? Please comment and resolve all discussions which are done now. Hi. There is one discussion still open and I commented on it. could you check?
konrad requested changes 2023-04-17 16:26:54 +00:00
konrad left a comment
Owner

I think we're already really far, most of these are nitpicks.

I think we're already really far, most of these are nitpicks.
@ -196,2 +199,4 @@
}
// does the oidc token contain well formed "vikunja_groups" through vikunja_scope
teamData, errs := getTeamDataFromToken(cl.VikunjaGroups, provider)
Owner

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

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

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.
viehlieb marked this conversation as resolved
@ -205,2 +229,4 @@
}
func AssignOrCreateUserToTeams(s *xorm.Session, u *user.User, teamData []models.OIDCTeamData) (oidcTeams []int64, err error) {
if len(teamData) > 0 {
Owner

Replace with

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

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

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

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
@ -207,0 +310,4 @@
}
func CreateTeamWithData(s *xorm.Session, teamData models.OIDCTeamData, u *user.User) (team *models.Team, err error) {
tea := &models.Team{
Owner

Please rename this to team.

Please rename this to `team`.
viehlieb marked this conversation as resolved
@ -207,0 +322,4 @@
// 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
Owner

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.

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.
viehlieb marked this conversation as resolved
@ -0,0 +1,96 @@
# Assign teams via oidc
This PR adds the functionality to assign users to teams via oidc.
Owner

Is this a note doc or a final document for end users (admins)?

Is this a note doc or a final document for end users (admins)?
Author
Contributor

it is supposed to be a final document for admins, with relevant information for admins.

it is supposed to be a final document for admins, with relevant information for admins.
Owner

Then please move it to docs/content/doc/setup and fix the comments, as stated below.

Then please move it to `docs/content/doc/setup` and fix the comments, as stated [below](https://kolaente.dev/vikunja/api/pulls/1393#issuecomment-50339).
viehlieb marked this conversation as resolved
Owner

Please rebase and resolve the conflicts. Please also not we don't keep generated swagger docs in the repo anymore, so you don't need to regenerate those.

Please rebase and resolve the conflicts. Please also not we don't keep generated swagger docs in the repo anymore, so you don't need to regenerate those.
viehlieb added 2 commits 2023-05-08 13:50:31 +00:00
viehlieb added 1 commit 2023-05-08 13:52:07 +00:00
work on openid to just start group workflow when teamData is available
Some checks failed
continuous-integration/drone/pr Build is failing
3fdbd53b3e
viehlieb added 1 commit 2023-05-08 14:05:01 +00:00
refactor unused function GetTeamsByName
Some checks failed
continuous-integration/drone/pr Build is failing
10ec7d977d
viehlieb added 36 commits 2023-05-08 14:44:41 +00:00
specify error on teams model, add more declarative error

specify error message on ErrOIDCTeamDoesNotExist
remove logic behind deleting last team_member
changug []models.Team to []*models.Team
viehlieb added 1 commit 2023-05-08 14:54:09 +00:00
fix rebase error
All checks were successful
continuous-integration/drone/pr Build is passing
223a6dc552
konrad reviewed 2023-05-08 20:14:01 +00:00
@ -1221,0 +1239,4 @@
// HTTPError holds the http error description
func (err ErrOIDCTeamDoesNotExist) HTTPError() web.HTTPError {
return web.HTTPError{HTTPCode: http.StatusNotFound, Code: ErrCodeTeamDoesNotExist, Message: "No Team with that name and valid property oidcId could be found."}
Owner

Error message should be No Team with that name and valid oidcId could be found. (same for the `Error()

Error message should be `No Team with that name and valid oidcId could be found.` (same for the `Error()
Owner

Error message should be No Team with that name and valid oidcId could be found. (same for the `Error()

Error message should be `No Team with that name and valid oidcId could be found.` (same for the `Error()
viehlieb marked this conversation as resolved
@ -122,1 +131,4 @@
// 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, oidcID string, teamName string) (team Team, err error) {
Owner

Please return a pointer to a

Please return a pointer to a
Author
Contributor

team i suppose.done, tx

team i suppose.done, tx
viehlieb marked this conversation as resolved
@ -206,0 +245,4 @@
if !exists {
err = tm.Create(s, u)
if err != nil {
log.Errorf("Could not assign %v to %v. %v", u.Username, team.Name, err)
Owner

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.
viehlieb marked this conversation as resolved
@ -429,0 +441,4 @@
}
// ErrCodeNoOpenIDEmailProvided holds the unique world-error code of this error
const ErrCodeOpenIDCustomScopeMalformed = 1022
Owner

Please add this error to the error doc

Please add this error to the error doc
viehlieb marked this conversation as resolved
konrad reviewed 2023-05-08 20:14:49 +00:00
@ -0,0 +1,98 @@
# Assign teams via oidc
Owner

Should this be a document intended for admins? Or only notes for this PR?

Should this be a document intended for admins? Or only notes for this PR?
Author
Contributor

Well actually a note for admins that are interested in using this feature.

Well actually a note for admins that are interested in using this feature.
Owner

Then please place it in the docs/content/doc/setup folder. And I think it needs some refinement, will add some comments.

Then please place it in the `docs/content/doc/setup` folder. And I think it needs some refinement, will add some comments.
viehlieb marked this conversation as resolved
viehlieb added 3 commits 2023-05-09 11:36:33 +00:00
konrad requested changes 2023-05-09 15:15:55 +00:00
@ -0,0 +4,4 @@
You need to configure your oidc provider as explained in the documentation below to make this feature work.
Tested with oidc provider authentik.
To distinguish between teams created in vikunja and teams generated via oidc, a string attribute for vikunja teams is introduced, which is called: *oidcID*
You should conigure your provider to send an oidcID to vikunja.
Owner

Please replace the whole paragraph with something like this:

Vikunja is capable of automatically adding users to a team based on a group defined in the oidc provider. If used, Vikunja will sync teams, automatically create new ones and make sure the members are part of the configured teams. Teams which only exist because they are generated from oidc attributes are not configurable in Vikunja.

See below for setup instructions.

To distinguish between teams created in Vikunja and teams generated automatically via oidc, generated teams have an `oidcID` assigned internally.
Please replace the whole paragraph with something like this: ``` Vikunja is capable of automatically adding users to a team based on a group defined in the oidc provider. If used, Vikunja will sync teams, automatically create new ones and make sure the members are part of the configured teams. Teams which only exist because they are generated from oidc attributes are not configurable in Vikunja. See below for setup instructions. To distinguish between teams created in Vikunja and teams generated automatically via oidc, generated teams have an `oidcID` assigned internally. ```
viehlieb marked this conversation as resolved
@ -0,0 +6,4 @@
To distinguish between teams created in vikunja and teams generated via oidc, a string attribute for vikunja teams is introduced, which is called: *oidcID*
You should conigure your provider to send an oidcID to vikunja.
## Setup
Owner

Replace with

## Setup for authentik
Replace with ``` ## Setup for authentik
viehlieb marked this conversation as resolved
@ -0,0 +47,4 @@
You should see "the description you entered in the oidc provider's admin area"
- Log in and go to teams.
- You will see "(sso: XXXXX)" written next to each team you were asigned through oidc.
Owner

Replace the whole paragraph with something like

To configure automatic team management through authentik, we assume you have already set up Authentik as an oidc provider for authentication with Vikunja.

To use Authentik's group assignment feature, follow these steps:

1. Edit [config.yml](https://kolaente.dev/vikunja/api/src/branch/main/config.yml.sample) to include scope: `openid profile email vikunja_scope`
2. Open `<your authentik url>/if/admin/#/core/property-mappings`
3. Create a new mapping called `vikunja_scope`. There is a field to enter python expressions that will be delivered with the oidc token.
4. Write a small script like this to add group information to `vikunja_scope`:


```python
groupsDict = {"vikunja_groups": []}
for group in request.user.ak_groups.all():
  groupsDict["vikunja_groups"].append({"name": group.name, "oidcID": group.num_pk})
return groupsDict

"""
output example:
{
    "vikunja_groups": [
        {
            "name": "team 1",
            "oidcID": 33349
        },
        {
            "name": "team 2",
            "oidcID": 35933
        }
    ]
}
"""

Now when you log into Vikunja via oidc there will be a list of scopes you are claiming from your oidc provider.
You should see the description you entered in the oidc provider's admin area.

  1. Log in and go to teams.
  2. You should see "(sso: XXXXX)" written next to each team you were asigned through oidc.
Replace the whole paragraph with something like ``` To configure automatic team management through authentik, we assume you have already set up Authentik as an oidc provider for authentication with Vikunja. To use Authentik's group assignment feature, follow these steps: 1. Edit [config.yml](https://kolaente.dev/vikunja/api/src/branch/main/config.yml.sample) to include scope: `openid profile email vikunja_scope` 2. Open `<your authentik url>/if/admin/#/core/property-mappings` 3. Create a new mapping called `vikunja_scope`. There is a field to enter python expressions that will be delivered with the oidc token. 4. Write a small script like this to add group information to `vikunja_scope`: ```python groupsDict = {"vikunja_groups": []} for group in request.user.ak_groups.all(): groupsDict["vikunja_groups"].append({"name": group.name, "oidcID": group.num_pk}) return groupsDict """ output example: { "vikunja_groups": [ { "name": "team 1", "oidcID": 33349 }, { "name": "team 2", "oidcID": 35933 } ] } """ ``` Now when you log into Vikunja via oidc there will be a list of scopes you are claiming from your oidc provider. You should see the description you entered in the oidc provider's admin area. 5. Log in and go to teams. 6. You should see "(sso: XXXXX)" written next to each team you were asigned through oidc. ```
viehlieb marked this conversation as resolved
@ -0,0 +53,4 @@
## IMPORTANT NOTES:
* **SSO/OIDC teams cannot be edited.**
* **It is crucial to call the element "vikunja_groups" since this is the name vikunja is looking for.**
Owner
* **It is required to call the custom scope "vikunja_groups" since this is the name vikunja is looking for.** 
``` * **It is required to call the custom scope "vikunja_groups" since this is the name vikunja is looking for.** ```
viehlieb marked this conversation as resolved
@ -0,0 +55,4 @@
* **It is crucial to call the element "vikunja_groups" since this is the name vikunja is looking for.**
* **Additionally, make sure to deliver an "oidcID" and a "name".**
Owner

Can you clarify this a little more?

Can you clarify this a little more?
viehlieb marked this conversation as resolved
@ -0,0 +59,4 @@
____________________________________________________________________________
Owner

Replace with --- (Will be expaned when rendered).

Replace with `---` (Will be expaned when rendered).
viehlieb marked this conversation as resolved
@ -0,0 +63,4 @@
## BEHAVIOR
*(.. examples for "team1" ..)*
Owner

Replace with

All examples assume one team called "team 1
Replace with ``` All examples assume one team called "team 1
viehlieb marked this conversation as resolved
viehlieb added 1 commit 2023-05-15 12:16:05 +00:00
change openid.md directory, modify openid.md
All checks were successful
continuous-integration/drone/pr Build is passing
a3a6b53128
Author
Contributor

hallo, ich habe gesehen, dass der alte PR geschlossen wurde.
Wie sieht es denn hiermit aus? Die Changes sind alle eingearbeitet..
lg

hallo, ich habe gesehen, dass der alte PR geschlossen wurde. Wie sieht es denn hiermit aus? Die Changes sind alle eingearbeitet.. lg
konrad requested changes 2023-10-18 15:21:33 +00:00
konrad left a comment
Owner

Found a few more things.

We also need a decision here: vikunja/api#1393 (comment)

Found a few more things. We also need a decision here: https://kolaente.dev/vikunja/api/pulls/1393#issuecomment-49350
@ -0,0 +5,4 @@
To distinguish between teams created in Vikunja and teams generated automatically via oidc, generated teams have an `oidcID` assigned internally.
## Setup for atuhentik
Owner

Typo: Should be Authentik.

Typo: Should be `Authentik`.
viehlieb marked this conversation as resolved
@ -0,0 +25,4 @@
```
```
output example:
Owner

Can you move the title to the outside of the example json?

Can you move the title to the outside of the example json?
viehlieb marked this conversation as resolved
@ -0,0 +42,4 @@
Now when you log into Vikunja via oidc there will be a list of scopes you are claiming from your oidc provider.
You should see the description you entered in the oidc provider's admin area.
Log in and go to teams.
Owner

Please change this so that it's clear that we're talking about Vikunja here.

Please change this so that it's clear that we're talking about Vikunja here.
viehlieb marked this conversation as resolved
@ -0,0 +43,4 @@
You should see the description you entered in the oidc provider's admin area.
Log in and go to teams.
You should see "(sso: XXXXX)" written next to each team you were asigned through oidc.
Owner

What's the XXXX part?

What's the `XXXX` part?
viehlieb marked this conversation as resolved
@ -0,0 +46,4 @@
You should see "(sso: XXXXX)" written next to each team you were asigned through oidc.
## IMPORTANT NOTES:
* **SSO/OIDC teams cannot be edited.**
Owner

Please don't make every paragraph here bold.

Please don't make every paragraph here bold.
viehlieb marked this conversation as resolved
@ -0,0 +54,4 @@
---
Owner

This is unnecessary, please remove it

This is unnecessary, please remove it
viehlieb marked this conversation as resolved
@ -0,0 +56,4 @@
---
## BEHAVIOR
Owner

What kind of behaviour? Please rename it to something more specific.

And please don't capitalize the heading, same for the other headings.

What kind of behaviour? Please rename it to something more specific. And please don't capitalize the heading, same for the other headings.
viehlieb marked this conversation as resolved
@ -0,0 +61,4 @@
*All examples assume one team called "team 1"*
1. *Token delivers team.name +team.oidcId and Vikunja team does not exist:* \
New team will be created called "team 1" with attribute oidcId: "33929"
Owner

Is the attribute called oidcId or oidcID? The Authentik example was using oidcId.

Is the attribute called `oidcId` or `oidcID`? The Authentik example was using `oidcId`.
Author
Contributor

in authentik it is called oidcID

in migration:

type teams20230104152903 struct {
	OidcID string `xorm:"varchar(250) null" maxLength:"250" json:"oidc_id"`
}

Therefore in the db it is called oidc_id

I agree, there is an inconsistency in the .md file.

I try to fit the naming to the context.
Do you have suggested changes in renaming?

in authentik it is called `oidcID` in migration: ``` type teams20230104152903 struct { OidcID string `xorm:"varchar(250) null" maxLength:"250" json:"oidc_id"` } ``` Therefore in the db it is called oidc_id I agree, there is an inconsistency in the .md file. I try to fit the naming to the context. Do you have suggested changes in renaming?
Owner

I think the name is fine, but please use the same one everywhere.

I think the name is fine, but please use the same one everywhere.
viehlieb marked this conversation as resolved
@ -123,0 +138,4 @@
Where("oidc_id = ? AND name = ?", oidcID, teamName).
Asc("id").
Limit(1).
Get(team)
Owner

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

`Get` will always return one entry, no need for `Asc` or `Limit`.
viehlieb marked this conversation as resolved
@ -197,0 +207,4 @@
//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)
Owner

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.
viehlieb marked this conversation as resolved
@ -197,0 +215,4 @@
}
errs = RemoveUserFromTeamsByIds(s, u, utils.NotIn(oldOidcTeams, oidcTeams))
for _, err := range errs {
log.Errorf("Found Error while signing out from teams %v", err)
Owner

Please don't call this "Sign out". That's a different thing.

Please don't call this "Sign out". That's a different thing.
viehlieb marked this conversation as resolved
@ -206,0 +255,4 @@
func RemoveUserFromTeamsByIds(s *xorm.Session, u *user.User, teamIDs []int64) (