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.
What does this do? Ensure the user is only member of teams provided by the oidc provider? What about teams created in Vikunja manually?
I feel like we should not delete a team here as that's pretty destructive. Maybe add a command to clean up empty teams later but I think we should not do that here.
What does this do? Ensure the user is only member of teams provided by the oidc provider? What about teams created in Vikunja manually?
I feel like we should not delete a team here as that's pretty destructive. Maybe add a command to clean up empty teams later but I think we should not do that here.
User as "team_member" will be removed via (user.id, team.id) for all teamIDs []int64.
If the user is the last member, the team is deleted.
Further explanation below:
deleting the team is not managed via oidc.
the token delivered by the oidc provider just contains information about the team memberships of the user.
Meaning:
team1 exists and was created through oidc (oidcId is set to something not null or "").
user1 is team_member of team1.
changes in oidc provider change team_membership of user1 >> team1 is not longer contained in oidc token for user1.
user1 logs in through oidc and is kicked out of team, which is not possible, if user1 is the last member of team1.
if user1 is last member >> team1 is deleted.
what is missing though:
a check whether user1 is in fact the last member of team1, then only delete team1.
User as "team_member" will be removed via `(user.id, team.id)` for all `teamIDs []int64`.
If the user is the last member, the team is deleted.
Further explanation below:
deleting the team is not managed via oidc.
the token delivered by the oidc provider just contains information about the team memberships of the user.
### Meaning:
* `team1` exists and was created through oidc (oidcId is set to something not null or "").
* `user1` is team_member of team1.
* changes in oidc provider change team_membership of `user1` >> `team1` is not longer contained in oidc token for `user1`.
* `user1` logs in through oidc and is kicked out of team, which is not possible, if `user1` is the last member of `team1`.
* if `user1` is last member >> `team1` is deleted.
what is missing though:
a check whether `user1` is in fact the last member of `team1`, then only delete `team1`.
user1 logs in through oidc and is kicked out of team, which is not possible, if user1 is the last member of team1.
Ah, now I understand how that came about. I still think we should not delete the team at that moment, better change the function to allow removing the last member in that case. There will probably be cases where a user is accidentally removed from the oicd claim, the team gets deleted and all shares with that team as well. Then the user is added again, and the shares need to be added again.
I don't know if this is only hypothetical, but it's something users will complain about when it happens.
It's fine to use this to sync teams and memberships but I feel like there's more to it.
> user1 logs in through oidc and is kicked out of team, which is not possible, if user1 is the last member of team1.
Ah, now I understand how that came about. I still think we should not delete the team at that moment, better change the function to allow removing the last member in that case. There will probably be cases where a user is accidentally removed from the oicd claim, the team gets deleted and all shares with that team as well. Then the user is added again, and the shares need to be added again.
I don't know if this is only hypothetical, but it's something users will complain about when it happens.
It's fine to use this to sync teams and memberships but I feel like there's more to it.
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)
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..
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.
Intermediate struct which only holds TeamName and OidcId Description.
It does not exist as a Team yet or better: it is the data accessible via oidc, which the Team struct is compared against.
Intermediate struct which only holds TeamName and OidcId Description.
It does not exist as a Team yet or better: it is the data accessible via oidc, which the Team struct is compared against.
Okay, but isn't it missing the json tags then? I'm not sure if I understood you correctly herre.
If it's only used for oidc data the name should reflect that. TeamData is too generic.
Okay, but isn't it missing the json tags then? I'm not sure if I understood you correctly herre.
If it's only used for oidc data the name should reflect that. `TeamData` is too generic.
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?
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.
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.
Not sure if this is a good solution. Imagine one membership fails to get removed while the others get through. Then there's inconsistent state and the whole thing is not atomic anymore. I think we should instead return on the first error and let the caller deal with it.
Not sure if this is a good solution. Imagine one membership fails to get removed while the others get through. Then there's inconsistent state and the whole thing is not atomic anymore. I think we should instead return on the first error and let the caller deal with it.
okay, but won't it reach inconsistent state anyways?
Imagine the user is in 3 groups.
tm is deleted for group 1
tm is not deleted for group 2, because user is last member
tm won't be deleted for group 3, where user is not last member and should be removed.
or do you suggest to catch ErrCannotDeleteLastTeamMember and ask to sign out manually from that specific group(s) and sign out from other groups anyway?
I think it is intended behavior to sign out from teams when possible, and to leave teams as they are, where error is found. Maybe giving a hint to leave manually because you are last user.
Did I understand you correctly?
okay, but won't it reach inconsistent state anyways?
Imagine the user is in 3 groups.
tm is deleted for group 1
tm is not deleted for group 2, because user is last member
tm won't be deleted for group 3, where user is not last member and should be removed.
or do you suggest to catch ErrCannotDeleteLastTeamMember and ask to sign out manually from that specific group(s) and sign out from other groups anyway?
I think it is intended behavior to sign out from teams when possible, and to leave teams as they are, where error is found. Maybe giving a hint to leave manually because you are last user.
Did I understand you correctly?
I think it is intended behavior to sign out from teams when possible, and to leave teams, where error is found.
What's your difference between "signing out from a team" and "leaving a team"? Aren't the two the same?
okay, but won't it reach inconsistent state anyways?
Not if the whole db transaction is rolled back.
> I think it is intended behavior to sign out from teams when possible, and to leave teams, where error is found.
What's your difference between "signing out from a team" and "leaving a team"? Aren't the two the same?
> okay, but won't it reach inconsistent state anyways?
Not if the whole db transaction is rolled back.
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
Ah so "leave a team" in this context is "not deleting / modifying it"? Not "A user leaves a team"?
I guess I'm just understanding it differently :)
if a member sits on a team as last member, then the member cannot leave other teams.
Why would it do that? That's not what should happen, but how would being the last member of one team prevent that user to leave another team?
where the user cannot be signed out because of
ErrCannotDeleteLastTeamMember{tm.TeamID, tm.UserID}
should be resolved manually then
But resolving this manually would mean the user needs to manually remove the team, which they cannot do because the team was created from openid?
> to leave teams as they are, where error is found
Ah so "leave a team" in this context is "not deleting / modifying it"? Not "A user leaves a team"?
I guess I'm just understanding it differently :)
> if a member sits on a team as last member, then the member cannot leave other teams.
Why would it do that? That's not what should happen, but how would being the last member of one team prevent that user to leave another team?
> where the user cannot be signed out because of
ErrCannotDeleteLastTeamMember{tm.TeamID, tm.UserID}
should be resolved manually then
But resolving this manually would mean the user needs to manually remove the team, which they cannot do because the team was created from openid?
if a member sits on a team as last member, then the member cannot leave other teams.
...is edited. I meant: the member cannot leave this team, if it is the last membership.
where the user cannot be signed out because of
ErrCannotDeleteLastTeamMember{tm.TeamID, tm.UserID}
should be resolved manually then
But resolving this manually would mean the user needs to manually remove the team, which they cannot do because the team was created from openid?
I think it is the responsibility of oidc admins to not let a team go blank without coordinating the potential destructive danger of it.
Therefore i would just delete the team + team_memberships.
> if a member sits on a team as last member, then the member cannot leave other teams.
...is edited. I meant: the member cannot leave this team, if it is the last membership.
> where the user cannot be signed out because of
ErrCannotDeleteLastTeamMember{tm.TeamID, tm.UserID}
should be resolved manually then
> But resolving this manually would mean the user needs to manually remove the team, which they cannot do because the team was created from openid?
I think it is the responsibility of _oidc admins_ to not let a team go blank without coordinating the potential destructive danger of it.
Therefore i would just delete the team + team_memberships.
I think it is the responsibility of oidc admins to not let a team go blank without coordinating the potential destructive danger of it. Therefore i would just delete the team + team_memberships.
Okay, that makes more sense.
> I think it is the responsibility of oidc admins to not let a team go blank without coordinating the potential destructive danger of it. Therefore i would just delete the team + team_memberships.
Okay, that makes more sense.
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.
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.
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?
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.
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.
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
}
```
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).
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.
returnweb.HTTPError{HTTPCode:http.StatusNotFound,Code:ErrCodeTeamDoesNotExist,Message:"No Team with that name and valid property oidcId could be found."}
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.
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.
```
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.
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.
Log in and go to teams.
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.
```
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?
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.
Please fix the lint and regenerate config docs.
@ -1194,0 +1201,4 @@
}
func (err ErrTeamsDoNotExist) Error() string {
return fmt.Sprintf("Team does not exist [Team Name: %v]", err.Name)
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.
@ -1194,0 +1205,4 @@
}
// ErrCodeTeamDoesNotExist holds the unique world-error code of this error
const ErrCodeTeamsDoNotExist = 6008
Please add the error to the error docs.
@ -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."}
Please change the message to "No team with that name exists"
done all the error stuff
@ -1194,0 +1224,4 @@
}
func (err ErrOIDCTeamsDoNotExistForUser) Error() string {
return fmt.Sprintf("No Oidc exists for User [User ID: %d]", err.UserID)
What OIDC does not exist? What does that mean?
@ -110,2 +110,4 @@
}
func (tm *TeamMember) CheckMembership(s *xorm.Session) (exists bool, err error) {
member, err := user2.GetUserByUsername(s, tm.Username)
Can you do this either with a join to avoid loading the user or at least use the "simple" function?
done.
@ -80,3 +83,3 @@
// TableName makes beautiful table names
func (*TeamMember) TableName() string {
func (TeamMember) TableName() string {
Why did you change this?
oha.
this was unintended.
@ -91,6 +94,12 @@ type TeamUser struct {
TeamID int64 `json:"-"`
}
type TeamData struct {
What is this used for? Please add a comment.
@ -123,0 +158,4 @@
if exists && err == nil {
return team, nil
}
return team, ErrTeamsDoNotExist{oidcID}
This error message should contain the name as well.
@ -123,0 +166,4 @@
Table("team_members").
Where("user_id = ? ", userID).
Join("RIGHT", "teams", "teams.id = team_members.team_id").
Where("teams.oidc_id != ?", "").
Can
teams.oidc_id
be null? Then you should check that as well.@ -123,0 +172,4 @@
if ts == nil || err != nil {
return ts, ErrOIDCTeamsDoNotExistForUser{userID}
}
return ts, nil
Why do you only load the ids here?
just ids are needed here to prevent overhead.
@ -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}
Why the extra variable?
manageAdmin func and this var are deleted from code, since not needed
manageAdmin func and this var are deleted from code, since not needed
@ -283,2 +338,4 @@
}
func (t *Team) ManageAdminRight(teamMember TeamMember, admin bool) {
// Insert the current user as member and admin
That's not what the function seems to be doing. Please change the name or comment.
@ -58,3 +60,3 @@
Oauth2Config *oauth2.Config `json:"-"`
}
type TeamData struct {
Didn't you declare this already somewhere else?
@ -66,0 +69,4 @@
Name string `json:"name"`
PreferredUsername string `json:"preferred_username"`
Nickname string `json:"nickname"`
Teams []string `json:"groups"`
Please rename this to `Groups
In the documentation you said only
vikunja_groups
are used. Does that mean this attribute is not needed at all?as a matter of fact, that's true.
removed
@ -66,0 +70,4 @@
PreferredUsername string `json:"preferred_username"`
Nickname string `json:"nickname"`
Teams []string `json:"groups"`
VikunjaGroups interface{} `json:"vikunja_groups"`
What is this used for? (While having `groups
@ -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)
Please remove this line.
@ -201,2 +213,4 @@
}
//find old teams for user through oidc
oldOidcTeams, _ := models.FindAllOidcTeamIDsForUser(s, u.ID)
Please don't ignore the error.
@ -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)
This should be an
Error
log message.@ -204,3 +249,4 @@
return auth.NewUserAuthTokenResponse(u, c, false)
}
func SignOutFromOrDeleteTeamsByID(s *xorm.Session, u *user.User, teamIDs []int64) {
What does this do? Ensure the user is only member of teams provided by the oidc provider? What about teams created in Vikunja manually?
I feel like we should not delete a team here as that's pretty destructive. Maybe add a command to clean up empty teams later but I think we should not do that here.
User as "team_member" will be removed via
(user.id, team.id)
for allteamIDs []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.user1
>>team1
is not longer contained in oidc token foruser1
.user1
logs in through oidc and is kicked out of team, which is not possible, ifuser1
is the last member ofteam1
.user1
is last member >>team1
is deleted.what is missing though:
a check whether
user1
is in fact the last member ofteam1
, then only deleteteam1
.Ah, now I understand how that came about. I still think we should not delete the team at that moment, better change the function to allow removing the last member in that case. There will probably be cases where a user is accidentally removed from the oicd claim, the team gets deleted and all shares with that team as well. Then the user is added again, and the shares need to be added again.
I don't know if this is only hypothetical, but it's something users will complain about when it happens.
It's fine to use this to sync teams and memberships but I feel like there's more to it.
okay, teams won't be deleted
@ -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)
Please return the error instead of logging only.
@ -207,0 +263,4 @@
log.Errorf("Cannot delete team %v", err)
}
}
}
This whole inner loop should be rewritten to something like this so that it's more idomatic go:
Also right now you'll only delete the team if deleting the member did not work, which does not really make sense.
changed behaviour to not delete team
@ -207,0 +270,4 @@
func getTeamDataFromToken(groups interface{}, provider *Provider) (teamData []TeamData, err error) {
teamData = []TeamData{}
if groups != nil {
el := groups.([]interface{})
Why not cast this to
[]map[string]interface{}
directly?no reason,
had claim VikunjaGroups more agnostic to specific data type.
now casts directly to []map[string]interface{}
@ -207,0 +285,4 @@
if team["oidcID"] != nil {
switch t := team["oidcID"].(type) {
case int64:
oidcID = fmt.Sprintf("%v", team["oidcID"])
Please use `strconv.FormatInt
done
@ -207,0 +290,4 @@
oidcID = string(team["oidcID"].(string))
case float64:
fl := float64(team["oidcID"].(float64))
oidcID = fmt.Sprintf("%v", fl)
Please use
strconv.FormatFloat
(not 100% sure if the function is really called that)@ -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{}
The return here will mean if one team claim is invalid, it will not try to validate any others. Is that what you intended?
changed behaviour to skipping the invalid team
@ -207,0 +311,4 @@
Description: teamData.Description,
OidcID: teamData.OidcID,
}
log.Debugf("Teams: %v", tea.OidcID)
Please make this debug message more descriptive.
@ -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{}
Please use
Here there is the information missing, what do you suggest?
Sorry, I meant
[]*models.Team{}
.@ -207,0 +336,4 @@
}
}
log.Debugf("Array: %v", te)
Please make this debug message more descriptive or remove it alltogether.
@ -277,1 +414,4 @@
}
// find the elements which appear in slice1,but not in slice2
func notIn(slice1 []int64, slice2 []int64) []int64 {
Can you move this to the
utils
package? Is there really nothing like this in the standard library?@ -278,0 +425,4 @@
break
}
}
// String not found. We add it to return slice
Don't you mean int?
@ -0,0 +1,99 @@
regarding:
Is this document final?
@ -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*
Where is that attribute placed? In the provider or Vikunja? How is it relevant for admins?
At that location, it is just an information as to what is happening at all.
Okay, we should just think of fixing it before merging the PR
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..
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.@ -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
Please link to the config option in the config doc.
@ -0,0 +50,4 @@
- You will see "(sso: XXXXX)" written next to each team you were asigned through oidc.
## IMPORTANT NOTES:
Please make this a list.
950_reworked_assign_teams_via_oidcto feat: assign users to teams via OIDC claims@ -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 |
Please add punctuation.
@ -1073,4 +1073,3 @@
}
// ErrTeamDoesNotExist represents an error where a team does not exist
type ErrTeamDoesNotExist struct {
Please don't remove the comment.
@ -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 {
Please add a comment, similar to the other errors.
@ -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{})
This should be
Exist(&TeamMember{})
.done
@ -112,0 +120,4 @@
err = s.
Where("team_id = ?", tm.TeamID).
Cols("user_id").
Find(&members)
If it's only counting, please use
Count()
.done
@ -91,6 +94,13 @@ type TeamUser struct {
TeamID int64 `json:"-"`
}
// TeamData is the relevant data for a team and is delivered by oidc token
What's the difference to the
Team
struct?Intermediate struct which only holds TeamName and OidcId Description.
It does not exist as a Team yet or better: it is the data accessible via oidc, which the Team struct is compared against.
Okay, but isn't it missing the json tags then? I'm not sure if I understood you correctly herre.
If it's only used for oidc data the name should reflect that.
TeamData
is too generic.No, the teamData has to be pulled out of token via
getTeamDataFromToken
Called it OIDCTeamData now.
@ -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)
Please don't ignore the errors.
not ignored now
af2cd7a42e
tod3ca751f4c
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?
5bd7451469
tofc89564ebc
@ -112,0 +117,4 @@
func (tm *TeamMember) GetMemberCount(s *xorm.Session) (memberCount int64, err error) {
member := TeamMember{}
memberCount, err = s.
Couldn't you just return the result here directly?
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.
@ -123,0 +136,4 @@
return teams, ErrTeamsDoNotExist{name}
}
var ts []*Team
Did you try passing the
Ups, I guess here is something missing
@ -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.
Please handle the error.
@ -123,0 +154,4 @@
exists, err := s.
Table("teams").
Where("oidc_id = ? AND name = ?", oidcID, teamName).
Get(&team)
What happens if there's more than one team with that combination (unlikely, but not handled)
I tend to just changing functionality to take the first team with that specific setting.
That sounds like it could work. Easiest way would be to add a
OrderBy
clause here.@ -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 {
I feel like the next 20 lines need better error handling - is it intended to not return when an error happens?
The rationale was to ensure users can still log in, even though the scope is malformed.
Therefore the Errors are logged instead of returning.
Also: It might be some groups with valid token details get through, some not.
We can also go for not signing in altogether.
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.
@ -207,0 +252,4 @@
return oidcTeams, err
}
func SignOutFromTeamsByID(s *xorm.Session, u *user.User, teamIDs []int64) (errs []error) {
Please call this something like
RemoveUserFromTeamsByID
so that it is clear how destructive this is.@ -207,0 +267,4 @@
err = tm.Delete(s, u)
// if you cannot delete the team_member
if err != nil {
errs = append(errs, err)
Not sure if this is a good solution. Imagine one membership fails to get removed while the others get through. Then there's inconsistent state and the whole thing is not atomic anymore. I think we should instead return on the first error and let the caller deal with it.
okay, but won't it reach inconsistent state anyways?
Imagine the user is in 3 groups.
tm is deleted for group 1
tm is not deleted for group 2, because user is last member
tm won't be deleted for group 3, where user is not last member and should be removed.
or do you suggest to catch ErrCannotDeleteLastTeamMember and ask to sign out manually from that specific group(s) and sign out from other groups anyway?
I think it is intended behavior to sign out from teams when possible, and to leave teams as they are, where error is found. Maybe giving a hint to leave manually because you are last user.
Did I understand you correctly?
What's your difference between "signing out from a team" and "leaving a team"? Aren't the two the same?
Not if the whole db transaction is rolled back.
edited
the edit is:
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.
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
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 :)
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?
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.
Okay, that makes more sense.
When I run it locally it seems to re-generate everything as it should.
There's an error in the CI:
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.
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.
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?
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)
The next 20 lines should be wrapped in a
if teamData != nil
so that the whole thing only gets executed when there is team data in the token. Otherwise this might generate error messages for setups not using teams via oicd.i removed this because of "cyclomatic complexity". But i can try to put it back in.
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.
@ -205,2 +229,4 @@
}
func AssignOrCreateUserToTeams(s *xorm.Session, u *user.User, teamData []models.OIDCTeamData) (oidcTeams []int64, err error) {
if len(teamData) > 0 {
Replace with
It could well be, that only a single team is received through the oidc token.
But
len(teamData) < 1
will only be true if there are 0 teams received?I should have phrased it better, this is equivalent:
yes, that's true of course. i do not know why i read it differently.
@ -207,0 +310,4 @@
}
func CreateTeamWithData(s *xorm.Session, teamData models.OIDCTeamData, u *user.User) (team *models.Team, err error) {
tea := &models.Team{
Please rename this to
team
.@ -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
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.@ -0,0 +1,96 @@
# Assign teams via oidc
This PR adds the functionality to assign users to teams via oidc.
Is this a note doc or a final document for end users (admins)?
it is supposed to be a final document for admins, with relevant information for admins.
Then please move it to
docs/content/doc/setup
and fix the comments, as stated below.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.
@ -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."}
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()@ -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) {
Please return a pointer to a
team i suppose.done, tx
@ -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)
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.@ -429,0 +441,4 @@
}
// ErrCodeNoOpenIDEmailProvided holds the unique world-error code of this error
const ErrCodeOpenIDCustomScopeMalformed = 1022
Please add this error to the error doc
@ -0,0 +1,98 @@
# Assign teams via oidc
Should this be a document intended for admins? Or only notes for this PR?
Well actually a note for admins that are interested in using this feature.
Then please place it in the
docs/content/doc/setup
folder. And I think it needs some refinement, will add some comments.@ -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.
Please replace the whole paragraph with something like this:
@ -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
Replace with
@ -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.
Replace the whole paragraph with something like
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.
@ -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.**
@ -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".**
Can you clarify this a little more?
@ -0,0 +59,4 @@
____________________________________________________________________________
Replace with
---
(Will be expaned when rendered).@ -0,0 +63,4 @@
## BEHAVIOR
*(.. examples for "team1" ..)*
Replace with
hallo, ich habe gesehen, dass der alte PR geschlossen wurde.
Wie sieht es denn hiermit aus? Die Changes sind alle eingearbeitet..
lg
Found a few more things.
We also need a decision here: vikunja/api#1393 (comment)
@ -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
Typo: Should be
Authentik
.@ -0,0 +25,4 @@
```
```
output example:
Can you move the title to the outside of the example json?
@ -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.
Please change this so that it's clear that we're talking about Vikunja here.
@ -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.
What's the
XXXX
part?@ -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.**
Please don't make every paragraph here bold.
@ -0,0 +54,4 @@
---
This is unnecessary, please remove it
@ -0,0 +56,4 @@
---
## BEHAVIOR
What kind of behaviour? Please rename it to something more specific.
And please don't capitalize the heading, same for the other headings.
@ -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"
Is the attribute called
oidcId
oroidcID
? The Authentik example was usingoidcId
.in authentik it is called
oidcID
in migration:
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?
I think the name is fine, but please use the same one everywhere.
@ -123,0 +138,4 @@
Where("oidc_id = ? AND name = ?", oidcID, teamName).
Asc("id").
Limit(1).
Get(team)
Get
will always return one entry, no need forAsc
orLimit
.@ -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)
Typo:
team
andoidc
should be lowercase.Please check the other log messages as well for typos, I've seen a few more like this.
@ -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)
Please don't call this "Sign out". That's a different thing.
@ -206,0 +255,4 @@
func RemoveUserFromTeamsByIds(s *xorm.Session, u *user.User, teamIDs []int64) (