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
continuous-integration/drone/pr Build is failing Details
82b7c14fed
fix lint
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
continuous-integration/drone/pr Build is failing Details
b607fea49d
change too generic name TeamData struct to OIDCTeamData
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
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
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
viehlieb added 1 commit 2023-05-08 14:05:01 +00:00
continuous-integration/drone/pr Build is failing Details
10ec7d977d
refactor unused function GetTeamsByName
viehlieb added 36 commits 2023-05-08 14:44:41 +00:00
d71ded051d add errors to error doc, rewrite error messages
specify error on teams model, add more declarative error

specify error message on ErrOIDCTeamDoesNotExist
8c1dcaea5f undo team gets deleted if user is last team member
remove logic behind deleting last team_member
f6ce982686 cast VikujGroups directly to []map[string]interface{}
changug []models.Team to []*models.Team
viehlieb added 1 commit 2023-05-08 14:54:09 +00:00
continuous-integration/drone/pr Build is passing Details
223a6dc552
fix rebase error
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
continuous-integration/drone/pr Build is passing Details
a3a6b53128
change openid.md directory, modify openid.md
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) (errs []error) {
errs = []error{}
for _, teamID := range teamIDs {
Owner

why not do this in one sql query?

why not do this in one sql query?
Author
Contributor

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

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

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

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 ```
viehlieb marked this conversation as resolved
@ -206,0 +310,4 @@
return teamData, errs
}
func CreateTeamWithData(s *xorm.Session, teamData models.OIDCTeamData, u *user.User) (team *models.Team, err error) {
Owner

What's the advantage of this method over team.Create?

What's the advantage of this method over `team.Create`?
Author
Contributor

i think it was cyclomatic complexity.

i think it was cyclomatic complexity.
konrad marked this conversation as resolved
viehlieb force-pushed 950_reworked_assign_teams_via_oidc from a3a6b53128 to 2377d816df 2023-10-27 14:24:22 +00:00 Compare
viehlieb added 1 commit 2023-10-27 14:30:41 +00:00
continuous-integration/drone/pr Build is failing Details
5c0e60ae9c
fixsomerebase errs
viehlieb added 4 commits 2023-11-07 14:27:20 +00:00
viehlieb added 2 commits 2023-11-08 11:44:36 +00:00
viehlieb added 1 commit 2023-11-08 13:50:30 +00:00
continuous-integration/drone/pr Build is failing Details
4f48cc6793
delete sso teams on login if empty
Contributor

Hey, I really love this PR and the feature you implement. I'm not a Go expert, still any chance I can help? Reviewing, testing? It would be incredible useful for our use case :)

Hey, I really love this PR and the feature you implement. I'm not a Go expert, still any chance I can help? Reviewing, testing? It would be incredible useful for our use case :)
Owner

@waza-ari It's close, only a very few small changes left.

Out of curiousity, what's your use-case?

@waza-ari It's close, only a very few small changes left. Out of curiousity, what's your use-case?
Contributor

@konrad Very happy to hear that, let me know if there's anything I can do, I'm happy to help.

Our use case is relatively simple - I'm leading a non profit org in Germany (a Makerspace, focussed on education and providing a free and open workshop for everyone) with roughly 400 members and we're looking for a tool to organise tasks within various groups (within the board, or within the various areas like woodworking, metal working and so on). There's responsible people for each of those that want to somehow organise their tasks and make them transparently available for all members.

We're using Keycloak for user management, as we're offering quite a lot of tools (our homepage, wiki, git, learning management, coding developer environment, even our wireless requires keycloak auth), and therefore its natural to connect the task management tool as well.

In keycloak we already have groups (and therefore both realm roles and sometimes client roles) associated to these groups, for example for member accounts, guest accounts, board members, area leads and so on and so forth. In Vikunja we'd maintain various projects and assign the Teams to them, but rather than manually maintaining memberships (especially as members come and go) it just not working.

Again, let me know if we can help in any way, and thanks for the awesome work you're doing!

@konrad Very happy to hear that, let me know if there's anything I can do, I'm happy to help. Our use case is relatively simple - I'm leading a non profit org in Germany (a Makerspace, focussed on education and providing a free and open workshop for everyone) with roughly 400 members and we're looking for a tool to organise tasks within various groups (within the board, or within the various areas like woodworking, metal working and so on). There's responsible people for each of those that want to somehow organise their tasks and make them transparently available for all members. We're using Keycloak for user management, as we're offering quite a lot of tools (our homepage, wiki, git, learning management, coding developer environment, even our wireless requires keycloak auth), and therefore its natural to connect the task management tool as well. In keycloak we already have groups (and therefore both realm roles and sometimes client roles) associated to these groups, for example for member accounts, guest accounts, board members, area leads and so on and so forth. In Vikunja we'd maintain various projects and assign the Teams to them, but rather than manually maintaining memberships (especially as members come and go) it just not working. Again, let me know if we can help in any way, and thanks for the awesome work you're doing!
Contributor

That being said I do have a question - looking through the implementation you're proposing to generate a custom scope based on assigned groups - is there a reason you're not using built in roles? IMHO this would be the more native OIDC implementation

That being said I do have a question - looking through the implementation you're proposing to generate a custom scope based on assigned groups - is there a reason you're not using built in roles? IMHO this would be the more native OIDC implementation
Owner

@waza-ari Nice, thanks for the overview - makes sense to wanting to provision the teams based on their role then. Are you already using Vikunja or are you planning to?

You'd be of help if you want to test with an unstable build once the feature once the PR is merged!

That being said I do have a question - looking through the implementation you're proposing to generate a custom scope based on assigned groups - is there a reason you're not using built in roles? IMHO this would be the more native OIDC implementation

You mean for the team assignment? I think that's a question for @viehlieb

Are the roles Keykloak-Specific or is that in the standard?

@waza-ari Nice, thanks for the overview - makes sense to wanting to provision the teams based on their role then. Are you already using Vikunja or are you planning to? You'd be of help if you want to test with an unstable build once the feature once the PR is merged! > That being said I do have a question - looking through the implementation you're proposing to generate a custom scope based on assigned groups - is there a reason you're not using built in roles? IMHO this would be the more native OIDC implementation You mean for the team assignment? I think that's a question for @viehlieb Are the roles Keykloak-Specific or is that in the standard?
Contributor

Are the roles Keykloak-Specific or is that in the standard?

I took roles for granted for a very long time already, but... looks like roles are actually Keycloak specific. I could've sworn they're standardised, but they're in fact not. Ignore what I wrote before :)

Nice, thanks for the overview - makes sense to wanting to provision the teams based on their role then. Are you already using Vikunja or are you planning to?

I've just set it up for some members and users to test, so far I've mostly received good feedback. We'll need to wait for this feature to get implemented, as right now it's hardly practicable to manually assign the people, especially considering that the users only get created once they login for the first time. I could probably pre-populate the database table, but I'd rather have the feature available and support if I can. I'll be more than happy to test once we're there.

Further I'll need to figure out how to configure the required claim in Keycloak. Once I do, I'll help writing the documentation for Keycloak of course

> Are the roles Keykloak-Specific or is that in the standard? I took roles for granted for a very long time already, but... looks like roles are actually Keycloak specific. I could've sworn they're standardised, but they're in fact not. Ignore what I wrote before :) > Nice, thanks for the overview - makes sense to wanting to provision the teams based on their role then. Are you already using Vikunja or are you planning to? I've just set it up for some members and users to test, so far I've mostly received good feedback. We'll need to wait for this feature to get implemented, as right now it's hardly practicable to manually assign the people, especially considering that the users only get created once they login for the first time. I could probably pre-populate the database table, but I'd rather have the feature available and support if I can. I'll be more than happy to test once we're there. Further I'll need to figure out how to configure the required claim in Keycloak. Once I do, I'll help writing the documentation for Keycloak of course
Author
Contributor

@waza-ari
you can do sth like
Screenshot from 2023-11-30 16-32-06.png

But i could not find out how to add codestyle
user.groups.each do etc.. to a hardcoded keycloak scope.

Maybe it is not possible..?

@waza-ari you can do sth like ![Screenshot from 2023-11-30 16-32-06.png](/attachments/632ab3d2-25f3-4aff-bcfe-a2c88f2a888c) But i could not find out how to add codestyle `user.groups.each do ` etc.. to a hardcoded keycloak scope. Maybe it is not possible..?
Contributor

@viehlieb creating custom mappers is possible, but quite complex. You need to add a custom javascript mapper as compiled JAR file. You can't even easily assign different hardcoded claims based on group membership.

That's the reason I was asking why specifically this format was chosen, as it won't be possible to implement this easily within Keycloak.

@viehlieb creating custom mappers is possible, but quite complex. You need to add a custom javascript mapper as compiled JAR file. You can't even easily assign different hardcoded claims based on group membership. That's the reason I was asking why specifically this format was chosen, as it won't be possible to implement this easily within Keycloak.
viehlieb force-pushed 950_reworked_assign_teams_via_oidc from 8cb3652a3e to c2dc6d0e39 2023-12-01 12:12:08 +00:00 Compare
Owner

@waza-ari Is there an easy way to supply the role from Keycloak to Vikunja when using this? Then maybe we could build something to make using it easier (in a follow-up PR).

@waza-ari Is there an easy way to supply the role from Keycloak to Vikunja when using this? Then maybe we could build something to make using it easier (in a follow-up PR).
Contributor

Hi, adding roles as a claim is super easy in Keycloak. You can use existing mappers for that, either Realm roles (which are used across the entire Realm / tenant) or client roles which are client specific. The claim itself can be named arbitrarily, and the value is just a list of role names, e.g.

["Team 1", "Team 2"]

This is then very easy to configure and you can easily configure role membership based on groups or based on individual member assignments.

@viehlieb could you quickly comment what the oidcId is needed for? If we could change the format to a simple list of strings, it's more flexible in terms of which OIDC providers can be supported.

If possible it might be better to change that now, otherwise it would be a breaking change for existing users.

image

Hi, adding roles as a claim is super easy in Keycloak. You can use existing mappers for that, either Realm roles (which are used across the entire Realm / tenant) or client roles which are client specific. The claim itself can be named arbitrarily, and the value is just a list of role names, e.g. `["Team 1", "Team 2"]` This is then very easy to configure and you can easily configure role membership based on groups or based on individual member assignments. @viehlieb could you quickly comment what the `oidcId` is needed for? If we could change the format to a simple list of strings, it's more flexible in terms of which OIDC providers can be supported. If possible it might be better to change that now, otherwise it would be a breaking change for existing users. ![image](/attachments/f49ccbf4-5aa8-4406-9347-c9321a6c3955)
189 KiB
konrad requested changes 2023-12-01 15:05:27 +00:00
@ -317,2 +317,4 @@
# The client secret used to authenticate Vikunja at the OpenID Connect provider.
clientsecret:
# The scope necessary to use oidc.
# If you want to use the Feature to create and assign to vikunja teams via oidc, you have to add the custom "vikunja_scope" and check [openid.md](https://kolaente.dev/vikunja/api/src/branch/main/pkg/modules/auth/openid/openid.md)
Owner

Please linke this to the docs, not the markdown file. The article will be available at https://vikunja.io/docs/openid/

Please linke this to the docs, not the markdown file. The article will be available at https://vikunja.io/docs/openid/
viehlieb marked this conversation as resolved
@ -0,0 +18,4 @@
```python
groupsDict = {"vikunja_groups": []}
Owner

Here it is called vikunja_groups and in the text before it's vikunja_scope. Should this be the same?

Here it is called `vikunja_groups` and in the text before it's `vikunja_scope`. Should this be the same?
Author
Contributor

one is the required scope, which has to be customized -> vikunja_scope
another one is a key for the groups within this scope -> vikunja_groups.

the scope attribute in vikunja should be pluralized into "scopes"

in authentik the attribute that is asked for is referred to as scope.
Screenshot from 2023-12-06 14-35-25.png

one is the required scope, which has to be customized -> vikunja_scope another one is a key for the groups within this scope -> vikunja_groups. the scope attribute in vikunja should be pluralized into "scopes" in authentik the attribute that is asked for is referred to as scope. ![Screenshot from 2023-12-06 14-35-25.png ](https://kolaente.dev/vikunja/api/issues/1393/attachments/941b6b69-6f92-4abe-8d1c-b23ab43e8626)
Owner

Can you add this explanation to the docs?

Can you add this explanation to the docs?
Author
Contributor

I changed the docs a little to clarify this.

5f521c0982

Not sure if there is more needed.

I changed the docs a little to clarify this. https://kolaente.dev/vikunja/vikunja/commit/5f521c0982aa220b687c915cd8b02a01d3c696ab Not sure if there is more needed.
@ -1054,3 +1053,3 @@
}
// ErrTeamAlreadyHasAccess represents an error where a team already has access to a project
// ErrTeamAlreadyHasAccess represents an error where a team already has access to a list/namespace
Owner

Please revert this.

Please revert this.
viehlieb marked this conversation as resolved
@ -198,0 +221,4 @@
if err != nil {
log.Errorf("Found error while leaving teams %v", err)
}
errors := RemoveEmptySSOTeams(s, teamIDsToLeave)
Owner

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

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..
konrad marked this conversation as resolved
@ -207,0 +289,4 @@
}
log.Debugf("Removing team_member with user_id %v from team_ids %v", u.ID, strings.Join(strSlice, ","))
_, err = s.Where("team_id IN (?) AND user_id = ?", strings.Join(strSlice, ","), u.ID).Delete(&models.TeamMember{})
Owner

Please use xorm's In function because that allows you to pass an int slice directly and avoids the need for the conversion you're doing.

Please use xorm's `In` function because that allows you to pass an int slice directly and avoids the need for the conversion you're doing.
viehlieb marked this conversation as resolved
@ -207,0 +300,4 @@
var name string
var description string
var oidcID string
if team["name"] != nil {
Owner

Please use _, exists := team["name"] to check if the field exists (exists is a boolean indicating whether the field exists). Same for the other fields.

Please use `_, exists := team["name"]` to check if the field exists (`exists` is a boolean indicating whether the field exists). Same for the other fields.
viehlieb marked this conversation as resolved
@ -132,6 +136,7 @@ func getProviderFromMap(pi map[string]interface{}) (provider *Provider, err erro
OriginalAuthURL: pi["authurl"].(string),
ClientSecret: pi["clientsecret"].(string),
LogoutURL: logoutURL,
Scope: scope,
Owner

Aren't we always expecting the scope to be vikunja_groups? Why make it configurable then instead of always appending it?

Aren't we always expecting the scope to be `vikunja_groups`? Why make it configurable then instead of always appending it?
Author
Contributor

If you have a look at:

3643ffe0d0/src/helpers/redirectToProvider.ts (L18)

You'll see the hardcoded scope.
But to receive more information instead of openid profile email from the oidc provider you'll need an extra scope.

There was a PR on the frontend explaining this issue.

vikunja/frontend#2749

This part makes sure there is always the "openid profile email" scope set - as it was before, hardcoded.. But if you want to add a custom scope in the config.yml, then you have to tell the provider in back and frontend to actually use it.

	scope, _ := pi["scope"].(string)
	if scope == "" {
		scope = "openid profile email"
	}
If you have a look at: https://kolaente.dev/vikunja/frontend/src/commit/3643ffe0d0357c89cb3517fafbb0c438188ac88d/src/helpers/redirectToProvider.ts#L18 You'll see the hardcoded scope. But to receive more information instead of openid profile email from the oidc provider you'll need an extra scope. There was a PR on the frontend explaining this issue. https://kolaente.dev/vikunja/frontend/pulls/2749 This part makes sure there is always the "openid profile email" scope set - as it was before, hardcoded.. But if you want to add a custom scope in the config.yml, then you have to tell the provider in back and frontend to actually use it. ``` scope, _ := pi["scope"].(string) if scope == "" { scope = "openid profile email" } ```
Owner

Yes it is hardcoded in the frontend, but why make it a manual config setting instead of passing it automatically from the api to the provider? That would allow us to get rid of the extra config variable.

Yes it is hardcoded in the frontend, but why make it a manual config setting instead of passing it automatically from the api to the provider? That would allow us to get rid of the extra config variable.
Author
Contributor

So you suggest to add:
Scope: "openid profile email vikunja_scope"

Anyway it has to be explained what "vikunja_scope" is and how it is used. I think it might be helpful for admins to use the scopes explicitly.
But I do not have a strong opinion on that

So you suggest to add: `Scope: "openid profile email vikunja_scope"` Anyway it has to be explained what "vikunja_scope" is and how it is used. I think it might be helpful for admins to use the scopes explicitly. But I do not have a strong opinion on that
Owner

Okay, now I got you. Sorry for the confusion. Let's keep it the way you intended, please add an example scope to the config.yml.sample file.

Okay, now I got you. Sorry for the confusion. Let's keep it the way you intended, please add an example scope to the `config.yml.sample` file.
Author
Contributor

Done, it is also linked to the explanation in openid.md

Done, it is also linked to the explanation in openid.md
konrad added 2 commits 2023-12-01 15:05:50 +00:00
continuous-integration/drone/pr Build is passing Details
c522842d0c
chore: refactor
viehlieb added 1 commit 2023-12-06 13:44:07 +00:00
Author
Contributor

@waza-ari

@viehlieb could you quickly comment what the oidcId is needed for? If we could change the format to a simple list of strings, it's more flexible in terms of which OIDC providers can be supported.

To distinguish between teams from oidc and teams from vikunja.

@waza-ari > @viehlieb could you quickly comment what the `oidcId` is needed for? If we could change the format to a simple list of strings, it's more flexible in terms of which OIDC providers can be supported. To distinguish between teams from oidc and teams from vikunja.
Contributor

To distinguish between teams from oidc and teams from vikunja.

I will admit that I didn't look into the code and the implementation, but wouldn't a simple flag be simpler? Having a simple list of strings for the teams is more easy to implement in other OIDC IDPs (like Keycloak)

> To distinguish between teams from oidc and teams from vikunja. I will admit that I didn't look into the code and the implementation, but wouldn't a simple flag be simpler? Having a simple list of strings for the teams is more easy to implement in other OIDC IDPs (like Keycloak)
Author
Contributor

To distinguish between teams from oidc and teams from vikunja.

I will admit that I didn't look into the code and the implementation, but wouldn't a simple flag be simpler? Having a simple list of strings for the teams is more easy to implement in other OIDC IDPs (like Keycloak)

Is there a way to restrict the groups from keycloak? This decision was taken especially for not using all groups of the oidc provider in vikunja and give a more fine grained choice for the admins.

But it is something we can think about next.

Idea would be to add the general "groups" claim to scope and starting the routines.

> > To distinguish between teams from oidc and teams from vikunja. > > I will admit that I didn't look into the code and the implementation, but wouldn't a simple flag be simpler? Having a simple list of strings for the teams is more easy to implement in other OIDC IDPs (like Keycloak) Is there a way to restrict the groups from keycloak? This decision was taken especially for not using all groups of the oidc provider in vikunja and give a more fine grained choice for the admins. But it is something we can think about next. Idea would be to add the general "groups" claim to scope and starting the routines.
Contributor

Is there a way to restrict the groups from keycloak? This decision was taken especially for not using all groups of the oidc provider in vikunja and give a more fine grained choice for the admins.

But it is something we can think about next.

Idea would be to add the general "groups" claim to scope and starting the routines.

In Keycloak you typically use roles for this. Roles can be global or client specific, last option would probably be better. The workflow is as follows:

  • Users are in Keycloak
  • Users can optionally be organised in Groups
  • Roles are created, either Realm / Tenant wide or Client wide
  • Users or Groups of Users are assigned to those roles
  • These roles are then mapped to a scope / claim and are thus included

So what you would do in this case:

  • On the Vikunja Client, we create client roles specific for Vinkunja
  • Then we create a mapper that maps the client roles to a certain claim, the name can be freely defined.

This way only these roles are mapped to the client we have good control what will be visible in Vikunja.

I'd be very surprised if the other IDPs don't offer something similar, that's quite generic approach. It would be more generic imho than the mapping that is used now.

> Is there a way to restrict the groups from keycloak? This decision was taken especially for not using all groups of the oidc provider in vikunja and give a more fine grained choice for the admins. > > But it is something we can think about next. > > Idea would be to add the general "groups" claim to scope and starting the routines. In Keycloak you typically use roles for this. Roles can be global or client specific, last option would probably be better. The workflow is as follows: - Users are in Keycloak - Users can optionally be organised in Groups - Roles are created, either Realm / Tenant wide or Client wide - Users or Groups of Users are assigned to those roles - These roles are then mapped to a scope / claim and are thus included So what you would do in this case: - On the Vikunja Client, we create client roles specific for Vinkunja - Then we create a mapper that maps the client roles to a certain claim, the name can be freely defined. This way only these roles are mapped to the client we have good control what will be visible in Vikunja. I'd be very surprised if the other IDPs don't offer something similar, that's quite generic approach. It would be more generic imho than the mapping that is used now.
Contributor

Hi all, is there anything I can do to support with this MR? We're still delaying the Vikunja introduction here (we're a charitable non-profit org wanting to use Vikunja for internal task coordination), as this is mandatory feature for us. I know its all our free time, just want to offer support if there's anything I can do :)

Hi all, is there anything I can do to support with this MR? We're still delaying the Vikunja introduction here (we're a charitable non-profit org wanting to use Vikunja for internal task coordination), as this is mandatory feature for us. I know its all our free time, just want to offer support if there's anything I can do :)
Owner

Hey @viehlieb, are you still interested in this?

Hey @viehlieb, are you still interested in this?
Author
Contributor

Hey @viehlieb, are you still interested in this?

Hey. Yes, sure.
So i would fix pkg/modules/auth/openid/openid_test.go
and then it might be ready to merge, right?

We agreed on keycloak next?
greets

> Hey @viehlieb, are you still interested in this? Hey. Yes, sure. So i would fix pkg/modules/auth/openid/openid_test.go and then it might be ready to merge, right? We agreed on keycloak next? greets
Owner

Yes, plus the handful of comments which are still unresolved. Once that's done, I'll give it another review and then we're ready to merge.

Yes, plus the handful of comments which are still unresolved. Once that's done, I'll give it another review and then we're ready to merge.
viehlieb added 1 commit 2024-02-23 11:39:18 +00:00
viehlieb force-pushed 950_reworked_assign_teams_via_oidc from 690302ac46 to 03fd257138 2024-02-23 11:50:26 +00:00 Compare
Member

Hi viehlieb!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1393-950reworkedassignteamsviaoidc--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi viehlieb! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://1393-950reworkedassignteamsviaoidc--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
viehlieb added 1 commit 2024-02-23 13:22:08 +00:00
continuous-integration/drone/pr Build is passing Details
6962e819a5
changed assert to require in openid test
viehlieb added 1 commit 2024-02-23 13:57:31 +00:00
continuous-integration/drone/pr Build is passing Details
5f521c0982
work on docs
Owner

Since the frontend and api repo were merged, can you add the changes from vikunja/frontend#3033 (comment) into this PR here?

Since the frontend and api repo were merged, can you add the changes from https://kolaente.dev/vikunja/frontend/pulls/3033#issuecomment-60214 into this PR here?
Contributor

Good morning,

after spending some time I think I came up with a way how to convince Keycloak to return a claim in the format you've been specifying here and would like to contribute both the script and documentation necessary to make it work. Unfortunately it's not straight forward, but I believe it should work just fine.

@viehlieb just for me to confirm: Am I right to assume that the oidcId is any varchar, 250 in length at max? This should be part of the documentation imho.

For now I'll place it in our Github, but once this PR is merged I'll extend the docs with instructions. Preferably I'd like to test my implementation, so once this is merged and available in unstable, please allow me 2-3 days before releasing and I'll extend the docs accordingly.

Regards
Daniel

Good morning, after spending some time I think I came up with a way how to convince Keycloak to return a claim in the format you've been specifying here and would like to contribute both the script and documentation necessary to make it work. Unfortunately it's not straight forward, but I believe it should work just fine. @viehlieb just for me to confirm: Am I right to assume that the oidcId is any varchar, 250 in length at max? This should be part of the documentation imho. For now I'll place it in our Github, but once this PR is merged I'll extend the docs with instructions. Preferably I'd like to test my implementation, so once this is merged and available in unstable, please allow me 2-3 days before releasing and I'll extend the docs accordingly. Regards Daniel
viehlieb added 1 commit 2024-03-01 13:24:27 +00:00
continuous-integration/drone/pr Build is failing Details
e7fc94d5b5
add frontend code from #3033
viehlieb added 1 commit 2024-03-01 13:41:07 +00:00
continuous-integration/drone/pr Build is failing Details
c696dfff39
restore pnpm.lock from main
konrad reviewed 2024-03-01 19:57:51 +00:00
konrad left a comment
Owner

Looks pretty good already!

I've tested this now from the start to the end, worked (almost) flawlessly.

Please also take a look at the failing CI.

Looks pretty good already! I've tested this now from the start to the end, worked (almost) flawlessly. Please also take a look at the failing CI.
@ -0,0 +27,4 @@
1. Edit [your config]({{< ref "config.md">}}) to include the following scopes: `openid profile email vikunja_scope`
2. Open `<your authentik url>/if/admin/#/core/property-mappings`
3. Create a new property mapping called `vikunja_scope` as scope mapping. There is a field `expression` to enter python expressions that will be delivered with the oidc token.
4. Write a small script like the following to add group information to `vikunja_scope`:
Owner

Please add to this guide how to add the scope to the Auth provider so that it actually shows up in Vikunja.

Please add to this guide how to add the scope to the Auth provider so that it actually shows up in Vikunja.
Owner

Now done.

Now done.
konrad marked this conversation as resolved
@ -109,0 +112,4 @@
| 6010 | 400 | There are no oidc teams found for the user. |
| ErrorCode | HTTP Status Code | Description |
Owner

These error codes are duplicated with the block above.

These error codes are duplicated with the block above.
Owner

Done.

Done.
konrad marked this conversation as resolved
Contributor

Hey there,

I've documented and published the Keycloak Custom Provider (OIDC mapper) here: https://github.com/makerspace-darmstadt/keycloak-vikunja-mapper

Feel free to steal whatever you want to the docs, or maybe just link to the repo. As there's a few things involved (the custom JS mapper, meta-info) I'm not sure it makes much sense to include everything here.

Regards
Daniel

Hey there, I've documented and published the Keycloak Custom Provider (OIDC mapper) here: https://github.com/makerspace-darmstadt/keycloak-vikunja-mapper Feel free to steal whatever you want to the docs, or maybe just link to the repo. As there's a few things involved (the custom JS mapper, meta-info) I'm not sure it makes much sense to include everything here. Regards Daniel
First-time contributor

thanks for the work, looking forward to the feature!

thanks for the work, looking forward to the feature!
konrad added 3 commits 2024-03-02 08:24:19 +00:00
konrad added 1 commit 2024-03-02 08:25:49 +00:00
continuous-integration/drone/pr Build is failing Details
c6ed01d2da
docs: add link to Keycloak mapper
Owner

Hey there,

I've documented and published the Keycloak Custom Provider (OIDC mapper) here: https://github.com/makerspace-darmstadt/keycloak-vikunja-mapper

Feel free to steal whatever you want to the docs, or maybe just link to the repo. As there's a few things involved (the custom JS mapper, meta-info) I'm not sure it makes much sense to include everything here.

Regards
Daniel

Awesome, I've added a link to it!

> Hey there, > > I've documented and published the Keycloak Custom Provider (OIDC mapper) here: https://github.com/makerspace-darmstadt/keycloak-vikunja-mapper > > Feel free to steal whatever you want to the docs, or maybe just link to the repo. As there's a few things involved (the custom JS mapper, meta-info) I'm not sure it makes much sense to include everything here. > > Regards > Daniel Awesome, I've added a link to it!
konrad added 1 commit 2024-03-02 08:30:25 +00:00
continuous-integration/drone/pr Build was killed Details
04939ea29b
chore: reset pnpm lock
konrad force-pushed 950_reworked_assign_teams_via_oidc from 04939ea29b to d7b5801549 2024-03-02 08:31:27 +00:00 Compare
konrad merged commit ed4da96ab1 into main 2024-03-02 08:47:12 +00:00
konrad deleted branch 950_reworked_assign_teams_via_oidc 2024-03-02 08:47:12 +00:00
Owner

Thanks everyone!

Thanks everyone!
Author
Contributor

Thanks everyone!

  • 1 & thanks as well (:
> Thanks everyone! + 1 & thanks as well (:
Sign in to join this conversation.
No description provided.