feat: assign users to teams via OIDC claims #1393
No reviewers
Labels
No Label
dependencies
duplicate
help wanted
invalid
kind/bug
kind/feature
needs reproduction
question
security
wontfix
No Milestone
No Assignees
5 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/vikunja#1393
Loading…
Reference in New Issue
No description provided.
Delete Branch "viehlieb/api:950_reworked_assign_teams_via_oidc"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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) (errs []error) {
errs = []error{}
for _, teamID := range teamIDs {
why not do this in one sql query?
You mean something like:
Yes, but with xorm:
went for:
_, err = s.Where("team_id IN (?) AND user_id = ?", strings.Join(strSlice, ","), u.ID).Delete(&models.TeamMember{})
That's still working agains the orm - makes things more complicated in the end.
Please use xorm's
In
function instead.Something like this might work as well:
@ -206,0 +310,4 @@
return teamData, errs
}
func CreateTeamWithData(s *xorm.Session, teamData models.OIDCTeamData, u *user.User) (team *models.Team, err error) {
What's the advantage of this method over
team.Create
?i think it was cyclomatic complexity.
a3a6b53128
to2377d816df
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 :)
@waza-ari It's close, only a very few small changes left.
Out of curiousity, what's your use-case?
@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!
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
@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!
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?
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 :)
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
@waza-ari
you can do sth like
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..?
@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.
8cb3652a3e
toc2dc6d0e39
@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).
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.
@ -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)
Please linke this to the docs, not the markdown file. The article will be available at https://vikunja.io/docs/openid/
@ -0,0 +18,4 @@
```python
groupsDict = {"vikunja_groups": []}
Here it is called
vikunja_groups
and in the text before it'svikunja_scope
. Should this be the same?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.
Can you add this explanation to the docs?
I changed the docs a little to clarify this.
5f521c0982
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
Please revert this.
@ -198,0 +221,4 @@
if err != nil {
log.Errorf("Found error while leaving teams %v", err)
}
errors := RemoveEmptySSOTeams(s, teamIDsToLeave)
Doing this every time a user logs in probably has performance implications but that's a problem to care about when we'll hit it in the wild.
only if it has teamIDsToLeave, but yeah I agree.
The problem is more the oidc structure itself, that you "have" to check for changes "everytime" you sign in..
@ -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{})
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.@ -207,0 +300,4 @@
var name string
var description string
var oidcID string
if team["name"] != nil {
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.@ -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,
Aren't we always expecting the scope to be
vikunja_groups
? Why make it configurable then instead of always appending it?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.
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.
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
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.Done, it is also linked to the explanation in openid.md
@waza-ari
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.
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:
So what you would do in this case:
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.
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 :)
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
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.
690302ac46
to03fd257138
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!
Since the frontend and api repo were merged, can you add the changes from vikunja/frontend#3033 (comment) into this PR here?
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
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`:
Please add to this guide how to add the scope to the Auth provider so that it actually shows up in Vikunja.
Now done.
@ -109,0 +112,4 @@
| 6010 | 400 | There are no oidc teams found for the user. |
| ErrorCode | HTTP Status Code | Description |
These error codes are duplicated with the block above.
Done.
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
thanks for the work, looking forward to the feature!
Awesome, I've added a link to it!
04939ea29b
tod7b5801549
Thanks everyone!