fix: OIDC team comparison should not include the team name #2152
No reviewers
Labels
No Label
dependencies
duplicate
help wanted
invalid
kind/bug
kind/feature
needs reproduction
question
security
wontfix
No Milestone
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/vikunja#2152
Loading…
Reference in New Issue
No description provided.
Delete Branch "waza-ari/vikunja:fix-team-creation"
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?
The change introduced in #2150 introduces a bug where a Team would be re-created every time a user logs in, as the check if a team already exists was based on both the unique
oidcID
and thename
. This PR proposes to only base the check on the ID, as this should be unique.Hi waza-ari!
Thank you for creating a PR!
I've deployed the frontend changes of this PR on a preview environment under this URL: https://2152-fix-team-creation--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 somewhere. The easiest to use is https://try.vikunja.io/.
This preview does not contain any changes made to the api, only the frontend.
Have a nice day!
@ -133,2 +132,3 @@
// GetTeamByOidcID returns a team matching the given oidc_id
// For oidc team creation oidcID and Name need to be set
func GetTeamByOidcIDAndName(s *xorm.Session, oidcID string, teamName string) (*Team, error) {
func GetTeamByOidcID(s *xorm.Session, oidcID string, teamName string) (*Team, error) {
Please remove the
teamName
parameter as well since it is now unused.I left it in there because it's currently passed to the
ErrOIDCTeamDoesNotExist
exception, and then used in various places in the code. If you prefer I can remove those occurrences, although it might be a better idea to do that together with your suggestion belowI think having the name in there doesn't hold value, but having the openid provider has. Probably best to combine this with adding the combination of the provider.
Well I just noticed the oidc id is not enough, it should actually also use the openid provider id as well… since the id is only guaranteed to be unique within that one provider. We should merge this PR before fixing that though.
That is a very good point, you're absolutely right. I can try to tackle this tomorrow, question is if we first want to merge this one or not.
Given this whole team sync feature is not released yet, we are in no rush to fix this quickly. If you plan to implement this, I'm fine waiting.
Some implementation notes:
pkg/modules/auth/openid/openid.go:198
.openid_issuer
or something like that. Docs for creating migrations here.@konrad I'll work on that today, I believe it should be reasonably straight forward. One item for discussion though:
Right now, whenever a Team is created, the user who logged in and triggered the creation becomes admin of that team. Effectively this means that whoever logs in first triggering the team creation, gets admin privileges for the team. This might not be desired.
Should we extend the OIDC structure to include some admin flag?
Having the admin status on such a team doesn't have any benefit because the team should not be managable in Vikunja at all. This should be changed so that all users are members only, without admins (not sure how much effort that is). Could be a follow-up PR though.
3a609d4d89
to9a20596bb0
9a20596bb0
toea04c6cbe3
@konrad thanks, you're absolutely right. The PR should work as is, avoiding duplicate teams being created and leveraging the issuer to compare against existing teams.
I've not found an easy way to avoid setting the admin flag to true, as its currently done within the Teams Create method, and simply adding another parameter with default value doesn't seem to be a thing in Go :D
@ -1192,3 +1192,3 @@
// ErrTeamDoesNotExist represents an error where a team does not exist
func (err ErrOIDCTeamDoesNotExist) Error() string {
return fmt.Sprintf("No team with that name and valid oidcId could be found. [Team Name: %v] [OidcID : %v] ", err.Name, err.OidcID)
return fmt.Sprintf("No team could be found for the given oidcId and issuer. [OidcID : %v] [Issuer Name: %v] ", err.OidcID, err.Issuer)
Nitpick: That's not the issuer name but the issuer id.
It looks like the issuer is not set for old teams. I propose two solutions:
Hm, that's a difficult one. The first approach will likely not work, as the issuer is used to find the team in the first place. We'd need to put in extra logic to check if there's already a team with the same oidcID but no issuer set. There's a small risk that someone's already using the unstable version with two providers though.
2 is probably possible, although I may need help with that. My Go skills are very limited
Any preferences on the approach from your perspective? Maybe also the question: old teams can only exist since the initial merge a couple of days ago, is that a real problem?
It's probably not a real problem. Maybe best to just ignore the problem and let users who are already using this (probably us two) delete the old teams manually with a
delete from teams where issuer is null
.I've noticed another odd thing in my own testing: Even though I added the
vikunja_groups
scope and mapping, Authentik does not return it properly. It does, however, return agroups
claim in the token with all groups the user is part of. Testing with GitLab, it returns agroups_direct
property in the token with all GitLab organizations I am part of. Not sure where that's coming from and where my own custom mapping went. Will investigate this more.What about setting it to false? Where did you see the admin value being set?
(As I said, this could be a follow-up)
Well that was a false alarm because I didn't rebuild my frontend and therefore it didn't pass the scopes back to Authentik when redirecting.
Its basically a tradeoff between a little pain for more advanced users now (with limited impact) or having extra logic in the code that'll probably sit there, do nothing in 99% of the cases and might cause questions in the future. So yeah I'd agree here, sounds sensible to me
Sadly I've absolutely no experience with Authentic. Did you configure the mapper to add it to the AT or ID token? If you want I can play around with Authentic a bit more.
In the original PR I commented that the structure that's currently required is a bit odd and probably doesn't come out of the box for basically any IdP (although admittedly I didn't express myself very well). For Gitlab I have absolutely no idea how to achieve this, as Gitlab doesn't allow any custom claims or custom mappers according to the docs.
As far as I can tell it's currently set automatically when creating a Team in the existing model. See
pkg/models/teams.go
L293ff:Assumed changing this to
false
would lead to teams created in the UI to not have an admin, so not sure that's straight forward to change.Anyway, since the sync also removes old teams when no one is part of that team any more, it seems like the problem of "old teams without an issuer remaining" is not a problem.
Now that you mention that - I do wonder if the issuer is taken into account correctly when deleting teams. Just to avoid a situation where a user logging in through provider A would cause teams created by provider B to be deleted - let me verify that real quick
I think that's fine because while it requires more work from the IdP, it also allows more flexibility. For example, Admins in Authentik are part of an "Authentik Default Admins" team, which is always present in the token claim. Using a custom mapper allows me to filter that out if I don't want to create it in Vikunja.
I think it's not. I'd rather move that into a cron job instead of doing it in the auth request, since that only makes that request longer.
Yeah the way to go here is probably to refactor that out in a new function
createNewTeam
or similar with an extra boolean parameter if the first user should be made admin.It should be fine as far as I can tell. Essentially
RemoveEmptySSOTeams
is only called for teams the user has actually left (that is, teams the user has been a member of). As the user can only be member of teams created by the same issuer, it implicitly makes sure nothing else is deleted.I've taken a crack at that, however not feeling very confident. Happy to revert and move into another PR if that's no good. Tests are passing locally and it seems to work (admin if I create it myself, no admin if created through OIDC)
It's quite good already! Please move this to a new PR anyway since that's not really what this PR set out to solve.
71d729dc01
to3e068f0142
Thanks!