Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

oidc: default team #535

Merged
merged 1 commit into from
Jan 25, 2024
Merged

Conversation

raboof
Copy link
Contributor

@raboof raboof commented Jan 11, 2024

The assignment of (a) default team(s) happens once during user provisioning, to give administrators a chance to change the teams for a user later.

The assignment of (a) default team(s) does not go through the indirection of an OIDC Group, and expects the administrator to directly specify the teams the user should be a member of by default. This is intentional, because otherwise it could be confusing what would happen when the mapping between OIDC Groups and Teams changes after a user was already provisioned. This also matches the UI, which primarily shows (and allows changes to) the 'Team membership' of a user on the 'OpenID Connect Users' page, rather than showing the OIDC Group membership.

Implements #411

@stevespringett
Copy link
Owner

@nscuro Any thoughts on this?

for (final String teamName : teamNames) {
Team team = getTeam(teamName);
if (team == null) {
LOGGER.debug("Unknown team " + teamName);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be logged as warn, and with a bit more context IMO. Users should be able to see when their configuration can't be applied as expected.

Something like:

Cannot add user foo to team bar, because no team with that name exists

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment on lines +238 to +240
} else {
// Only apply default teams during auto-provisioning, not on later updates:
return qm.addUserToTeams(user, config.getPropertyAsList(Config.AlpineKey.OIDC_TEAMS_DEFAULT));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing it like this means that the default teams will be wiped upon next login, assuming team synchronization is enabled. So if the user logs out and in again, or simply switches to another browser / device, they will be back to no teams again.

There likely needs to be some additional logic in synchronizeTeamMembership that accounts for default teams.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When team synchronization is enabled, the default teams are not taken into account at all.

The idea was that if you want team synchronization, you should manage your teams in your identity infrastructure. Only when you don't have team synchronization, you can use the default teams to give users initial/default authorizations.

Copy link
Collaborator

@nscuro nscuro Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for clarifying. I think I misunderstood what was being requested in #411 - I assumed it was about having a "catch-all" group for users who don't have any groups assigned yet in the IdP. Seems I missed the

For organisations that are happy to manage users within an application

part of the original issue description. Makes sense to me then.

@raboof raboof force-pushed the oidc_default_team branch from 1338a05 to 1c8554b Compare January 24, 2024 09:09
The assignment of (a) default team(s) happens once during user provisioning,
to give administrators a chance to change the teams for a user later.

The assignment of (a) default team(s) does not go through the indirection of
an OIDC Group, and expects the administrator to directly specify the teams
the user should be a member of. This is intentional, because otherwise it
could be confusing what would happen when the mapping between OIDC Groups
and Teams changes after a user was already provisioned. This also matches
the UI, which primarily shows (and allows changes to) the 'Team membership'
of a user on the 'OpenID Connect Users' page, rather than the OIDC Group
membership.

Fixes stevespringett#411
@raboof raboof force-pushed the oidc_default_team branch from 1c8554b to 18ade6e Compare January 24, 2024 09:10
Copy link
Collaborator

@nscuro nscuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @raboof!

@stevespringett, this is good to merge.

@stevespringett stevespringett merged commit 0c2f6c9 into stevespringett:master Jan 25, 2024
2 checks passed
nscuro added a commit to nscuro/dependency-track that referenced this pull request Jan 28, 2024
Introduces:

* Ability to assign default groups to OIDC users (stevespringett/Alpine#535)
* Tracking of `created` and `lastUsed` timestamps for API keys (stevespringett/Alpine#537)
* Addition of `comment` field to API keys (stevespringett/Alpine#537)

Closes DependencyTrack#1068
Fixes DependencyTrack#1556
Closes DependencyTrack#3349

Signed-off-by: nscuro <[email protected]>
mikael-carneholm-2-wcar pushed a commit to mikael-carneholm-2-wcar/dependency-track that referenced this pull request Mar 15, 2024
Introduces:

* Ability to assign default groups to OIDC users (stevespringett/Alpine#535)
* Tracking of `created` and `lastUsed` timestamps for API keys (stevespringett/Alpine#537)
* Addition of `comment` field to API keys (stevespringett/Alpine#537)

Closes DependencyTrack#1068
Fixes DependencyTrack#1556
Closes DependencyTrack#3349

Signed-off-by: nscuro <[email protected]>
Signed-off-by: Mikael Carneholm <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants