AUTH-8: Add group synchronization from OIDC providers #87

stlaz · 2021-09-24T08:51:52Z

This PR adds group synchronization for OIDC providers as described in the Synchronizing User Groups on Login enhancement.

The usermappers normally used to create users are wrapped by a delegator that first attempts to perform the usermapping strategy and only after its success it attempts to add/remove the user in appropriate groups.

stlaz · 2021-09-24T08:52:16Z

/cc @s-urbaniak @ibihim

stlaz · 2021-09-24T08:55:38Z

/hold
for some reason the bot did not mark the PR as WIP, adding hold manually

stlaz · 2021-09-24T10:24:59Z

/hold cancel
there we go

ibihim

Great PR. I can't yet understand everything, but I hope my comments help you nevertheless.

ibihim · 2021-09-28T09:10:48Z

pkg/cmd/oauth-server/server.go

@@ -33,7 +33,7 @@ func RunOsinServer(osinConfig *osinv1.OsinServerConfig, stopCh <-chan struct{})
 		return errors.New("osin server requires non-empty oauthConfig")
 	}

-	oauthServerConfig, err := newOAuthServerConfig(osinConfig)
+	oauthServerConfig, postStartHooks, err := newOAuthServerConfig(osinConfig)


Wouldn't it be better, if postStartHooks would be returned from a dedicated function or being part of the oauthServerConfig (if they config something)?

I wouldn't anticipate to retrieve hooks, when calling for a config.

The main point, as far as I can tell (and this is not much, as I am a newbie and I am looking from an abstract and not holistic view), we are handing the definition of the hooks down to reach the informer. So I would assume that we could create the informer on this level and hand it down to the config and a dedicated make-hooks function.

This is the same how the postStartHooks are being passed through the other apiservers (oauth-, openshift- and kube-apiservers).

I don't think return of postStartHooks is actually that surprising here. You're generating configuration for the apiserver, and you want the apiserver to run certain actions that make the configuration work properly.

If it is idiomatic, let keep doing it that way 😄

In order to understand the idiomatic attempt to handling postStartHooks I looked into kubernetes/apiserver and openshift/openshift-apiserver, but couldn't find a similar pattern. In the former it is part of the Server and injected and in the later it is part of the Config.

Hmm, I think my point was more about delivering the postStartHooks to the inner GenericAPIServer of the server object,

I can hide the hooks in the config structure if you want, and use it while setting up the OAuthServer struct in the completedOAuthConfig.New method if you want.

moved the postStartHooks to the config

pkg/oauthserver/auth.go

pkg/api/types.go

pkg/groupmapper/groupmapper.go

ibihim · 2021-09-28T12:26:19Z

pkg/groupmapper/groupmapper.go

+// FIXME: add retries
+// FIXME: use idpName
+func (m *UserGroupsMapper) removeUserFromGroupWithRetries(idpName, username, group string) error {
+	updatedGroup, err := m.groupsClient.Get(context.TODO(), group, metav1.GetOptions{})


Shouldn't we bubble up the ctx?

the context is unfortunately not wired through the oauth-server, and I fear that to really do it properly we would have to also implement openshift/osin#82

on Sep 18, 2015 👀

Ok, I would mark the suggestion as too big for the issue.

Do we have refactoring opportunities?

pkg/groupmapper/groupmapper.go

stlaz · 2021-10-05T11:42:05Z

/test verify
looks suspiciously like failed infra

stlaz · 2021-10-06T12:50:19Z

/hold
contains fake bumps

s-urbaniak · 2021-10-12T07:24:03Z

pkg/oauthserver/auth.go

+	userIdentityMapping userclient.UserIdentityMappingInterface,
+	method identitymapper.MappingMethodType,
+) (api.UserIdentityMapper, error) {
+	userMapper, err := identitymapper.NewIdentityUserMapper(


as far as i see NewIdentityUserMapper is unused, is there any reason to keep this separate constructor? I think we are instanciating with groups only, no?

We are wrapping the UserIdentityWrapper result of the NewIdentityUserMapper with a groups mapper. I think it's worth having it separate to keep the logical wrapper -> wrapped relationship as each of these mappers are serving a slightly different purpose.

stlaz · 2021-10-13T13:20:04Z

now with real bumps!

ibihim · 2021-10-13T12:45:57Z

pkg/oauthserver/auth.go

+	groups userclient.GroupInterface,
+	groupsLister userlisterv1.GroupLister,
+	userIdentityMapping userclient.UserIdentityMappingInterface,
+	method identitymapper.MappingMethodType,


Nit: quite some amount of params. At some point we should consider switching to a config / functional options.

ibihim · 2021-10-13T13:48:52Z

pkg/cmd/oauth-server/server.go

@@ -33,7 +33,7 @@ func RunOsinServer(osinConfig *osinv1.OsinServerConfig, stopCh <-chan struct{})
 		return errors.New("osin server requires non-empty oauthConfig")
 	}

-	oauthServerConfig, err := newOAuthServerConfig(osinConfig)
+	oauthServerConfig, postStartHooks, err := newOAuthServerConfig(osinConfig)


In order to understand the idiomatic attempt to handling postStartHooks I looked into kubernetes/apiserver and openshift/openshift-apiserver, but couldn't find a similar pattern. In the former it is part of the Server and injected and in the later it is part of the Config.

pkg/groupmapper/groupmapper.go

ibihim · 2021-10-13T18:52:57Z

pkg/oauthserver/oauth_apiserver.go

@@ -43,7 +49,7 @@ func init() {

 // TODO we need to switch the oauth server to an external type, but that can be done after we get our externally facing flag values fixed
 // TODO remaining bits involve the session file, LDAP util code, validation, ...
-func NewOAuthServerConfig(oauthConfig osinv1.OAuthConfig, userClientConfig *rest.Config, genericConfig *genericapiserver.RecommendedConfig) (*OAuthServerConfig, error) {
+func NewOAuthServerConfig(oauthConfig osinv1.OAuthConfig, userClientConfig *rest.Config, genericConfig *genericapiserver.RecommendedConfig) (*OAuthServerConfig, map[string]genericapiserver.PostStartHookFunc, error) {


It would be cool if we could at some point trim down the function 😄

ibihim · 2021-10-13T19:16:57Z

pkg/oauthserver/oauth_apiserver.go

-	return ret, nil
+	postStartHooks := map[string]genericapiserver.PostStartHookFunc{
+		"openshift.io-StartUserInformer": func(ctx genericapiserver.PostStartHookContext) error {
+			go userInformer.Start(ctx.StopCh)


Do we need to start it as a Go routine? The Start method seems to iterate over the internal informers and start a Go routine too.

https://github.com/kubernetes/apiserver/blob/master/pkg/server/config.go#L664

I think it might be better to make sure the call is non-blocking if we don't own the implementation, but I can change it to not use the goroutine if you want.

It would be interesting to know if this Go routines will stay alive for as long as the child-Go routines are alive.

If the minimal blocking, while iterating through the for-loop could be more problematic than an additional Go-routine, then I am fine 😄

Wrap usermappers with a groupmapper that attempts to add a user to groups that they are not a part of or remove them from groups for the specific provider.

stlaz · 2021-10-18T10:00:53Z

/hold cancel

ibihim

/lgtm

ibihim · 2021-10-18T13:04:41Z

pkg/groupmapper/groupmapper.go

+					Name: group,
+					Annotations: map[string]string{
+						fmt.Sprintf(groupSyncedKeyFmt, idpName): "synced",
+						groupGeneratedKey:                       "true",


Nit:

Sorry, I just now realized this. Would it make sense to make "synced" and "true" constants?

openshift-ci · 2021-10-18T13:36:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ibihim, stlaz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [stlaz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2021-10-18T13:41:40Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-10-18T14:17:37Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci bot requested review from ibihim, s-urbaniak, mfojtik and sttts September 24, 2021 08:52

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 24, 2021

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 24, 2021

stlaz changed the title ~~WIP: AUTH-8: Add group synchronization from OIDC providers~~ [WIP] AUTH-8: Add group synchronization from OIDC providers Sep 24, 2021

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 24, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 24, 2021

stlaz force-pushed the groupsync_oidc branch 6 times, most recently from e4c60b0 to a8b4672 Compare September 27, 2021 15:13

ibihim reviewed Sep 28, 2021

View reviewed changes

stlaz force-pushed the groupsync_oidc branch 2 times, most recently from 74983be to 81db104 Compare October 5, 2021 11:33

stlaz changed the title ~~[WIP] AUTH-8: Add group synchronization from OIDC providers~~ AUTH-8: Add group synchronization from OIDC providers Oct 5, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 5, 2021

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2021

stlaz mentioned this pull request Oct 6, 2021

AUTH-8: Move usercache from the oauth-apiserver to lib-go openshift/library-go#1223

Merged

stlaz force-pushed the groupsync_oidc branch from 81db104 to e656082 Compare October 11, 2021 10:58

s-urbaniak reviewed Oct 12, 2021

View reviewed changes

stlaz force-pushed the groupsync_oidc branch from e656082 to a285157 Compare October 13, 2021 13:19

bump o/api to get the openidclaims.groups; bump lib-go to get usercache

d51ee1b

ibihim reviewed Oct 13, 2021

View reviewed changes

stlaz force-pushed the groupsync_oidc branch from a285157 to 76a1b41 Compare October 15, 2021 11:44

stlaz added 2 commits October 15, 2021 13:44

Add groupsynchronization

9b7ab80

Wrap usermappers with a groupmapper that attempts to add a user to groups that they are not a part of or remove them from groups for the specific provider.

more deps for tests

76a1b41

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2021

ibihim approved these changes Oct 18, 2021

View reviewed changes

openshift-ci bot assigned ibihim Oct 18, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2021

openshift-merge-robot merged commit 6eb7ac3 into openshift:master Oct 18, 2021

LucienBrule mentioned this pull request Feb 9, 2022

Gitlab OAuth Group Policy #99

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AUTH-8: Add group synchronization from OIDC providers #87

AUTH-8: Add group synchronization from OIDC providers #87

stlaz commented Sep 24, 2021

stlaz commented Sep 24, 2021

stlaz commented Sep 24, 2021

stlaz commented Sep 24, 2021

ibihim left a comment

ibihim Sep 28, 2021

stlaz Oct 1, 2021

ibihim Oct 4, 2021

ibihim Oct 13, 2021

stlaz Oct 14, 2021

stlaz Oct 15, 2021

ibihim Sep 28, 2021

stlaz Oct 1, 2021

s-urbaniak Oct 13, 2021

ibihim Oct 14, 2021 •

edited

Loading

stlaz commented Oct 5, 2021

stlaz commented Oct 6, 2021

s-urbaniak Oct 12, 2021

stlaz Oct 13, 2021

stlaz commented Oct 13, 2021

ibihim Oct 13, 2021

ibihim Oct 13, 2021

ibihim Oct 13, 2021

ibihim Oct 13, 2021 •

edited

Loading

stlaz Oct 14, 2021

ibihim Oct 14, 2021

stlaz commented Oct 18, 2021

ibihim left a comment

ibihim Oct 18, 2021

openshift-ci bot commented Oct 18, 2021

openshift-bot commented Oct 18, 2021

openshift-bot commented Oct 18, 2021

AUTH-8: Add group synchronization from OIDC providers #87

AUTH-8: Add group synchronization from OIDC providers #87

Conversation

stlaz commented Sep 24, 2021

stlaz commented Sep 24, 2021

stlaz commented Sep 24, 2021

stlaz commented Sep 24, 2021

ibihim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibihim Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

stlaz commented Oct 5, 2021

stlaz commented Oct 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlaz commented Oct 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibihim Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlaz commented Oct 18, 2021

ibihim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 18, 2021

openshift-bot commented Oct 18, 2021

openshift-bot commented Oct 18, 2021

ibihim Oct 14, 2021 •

edited

Loading

ibihim Oct 13, 2021 •

edited

Loading