collective / pas.plugins.oidc

PAS plugin for OpenID Connect authentication
Other
3 stars 11 forks source link

Implement minimal group provisioning #17

Closed datakurre closed 1 year ago

datakurre commented 1 year ago

We'd like to implement provisioning of groups directly from id token / user info, if it includes list of group ids:

  1. create missing groups
  2. tag groups to be "managed" when they receive members from plugin
  3. remove member from tagged group when user info no longer includes membership

This is opinionated approach, so I'd request for comments, before polishing with requested changes, changelog and tests.

For example, should there be more granular configuration? Separate toggles for group creation and membership management? Customizable user info attribute for groups list? Customizable group property used in tagging? Something else?

erral commented 1 year ago

I would at least add an option to set which property should be checked to get the groups as it is done in #16 for user information.

erral commented 1 year ago

See #20

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 4346480969


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/pas/plugins/oidc/plugins.py 4 24 16.67%
<!-- Total: 4 24 16.67% -->
Totals Coverage Status
Change from base Build 4282097293: -2.5%
Covered Lines: 229
Relevant Lines: 408

💛 - Coveralls
erral commented 1 year ago

I have rebased this, to have at least the tests green :)

erral commented 1 year ago

@mauritsvanrees @mamico what do you think about this?

I am not sure if this should be implemented like this or like another plugin, we can discuss it...

mamico commented 1 year ago

@mauritsvanrees @mamico what do you think about this?

I am not sure if this should be implemented like this or like another plugin, we can discuss it...

@erral I like the idea of managing groups with the OIDC claim, but currently I don't have a real use case to compare it to. The only thought I would have is whether it's better to use a property of the groups or a naming convention to distinguish OIDC-managed groups from others.

erral commented 1 year ago

@mauritsvanrees @mamico what do you think about this? I am not sure if this should be implemented like this or like another plugin, we can discuss it...

@erral I like the idea of managing groups with the OIDC claim, but currently I don't have a real use case to compare it to. The only thought I would have is whether it's better to use a property of the groups or a naming convention to distinguish OIDC-managed groups from others.

Yeah, I didn't even know that it was possible to add properties to groups :smile:

I have a usecase, the one explained in the blog post where we can use a user property coming from Google (in this case the property is hd) to identify users and create a group with them.

datakurre commented 1 year ago

We also though whether we should prefix the group names to separate provisioned groups from local groups, but eventually I chose to use property to avoid confusion caused by the prefix and naming decision for the prefix.