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

Extends provisioning of users #502

Merged

Conversation

MarvinOehlerkingCap
Copy link
Contributor

This PR adds functionality for auto provision user when accessing API and WebDav with Bearer token.

New settings for provider:

  • groupProvisioning (This will create and update the users groups depending on the groups claim in the id token.)
  • mappingGroups (Claim in the idToken which will be used for provisiong of groups.)
  • providerBasedId (Use provider identifier as prefix for ids.)
  • bearerProvisioning (This automatically provisions the user, when sending API and WebDav Requests with a Bearer token. Auto provisioning and Bearer token check have to be activated for this to work.)

Some code improvements:

  • Moved provisioning code from Controller to a separate service

@kronn
Copy link

kronn commented Oct 5, 2022

This looks like everything I want from this plugin. Especially the ability to pass groups and have them updated later is high on my wishlist. Is there anything I can do to help this along?

Copy link

@kronn kronn left a comment

Choose a reason for hiding this comment

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

Looks good to me, I like especially the group-updating.

Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

Wow this is awesome.

A few remarks though.

I'll try this in a real setup soon.

composer.json Outdated Show resolved Hide resolved
lib/Service/IdService.php Outdated Show resolved Hide resolved
lib/Service/ProvisioningService.php Show resolved Hide resolved
lib/Service/ProvisioningService.php Show resolved Hide resolved
lib/Service/ProvisioningService.php Outdated Show resolved Hide resolved
lib/Service/ProvisioningService.php Outdated Show resolved Hide resolved
lib/User/Backend.php Outdated Show resolved Hide resolved
src/components/SettingsForm.vue Outdated Show resolved Hide resolved
src/components/SettingsForm.vue Outdated Show resolved Hide resolved
@julien-nc
Copy link
Member

Oh and could you rebase on master?

@MarvinOehlerkingCap MarvinOehlerkingCap requested review from julien-nc and removed request for juliusknorr October 14, 2022 13:41
@julien-nc
Copy link
Member

julien-nc commented Oct 18, 2022

@MarvinOehlerkingCap Thanks for the changes. Missing things:

  • @julien-nc to try it in a real environment
  • rebase on master

Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

We're getting there. Apart from the inline comment, it looks good to me.

Could you rebase on master?
You will have to adjust to the @nextcloud/vue bump with the new component names.
I can do the rebase if you prefer.

lib/Service/ProvisioningService.php Show resolved Hide resolved
@julien-nc
Copy link
Member

Side note: I got some 500 responses from Keycloak when setting user attributes of type JSON that don't have the object keys between parenthesis. Not our problem it but might be tricky to debug for admins.

- groupProvisioning (This will create and update the users groups depending on the groups claim in the id token.)
- mappingGroups (Claim in the idToken which will be used for provisiong of groups.)
- providerBasedId (Use provider identifier as prefix for ids.)
- bearerProvisioning (This automatically provisions the user, when sending API and WebDav Requests with a Bearer token. Auto provisioning and Bearer token check have to be activated for this to work.)
Some code improvements:
-Moved provisioning code from Controller to a separate service

Signed-off-by: Marvin Öhlerking <marvin.oehlerking@capgemini.com>
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

Thanks for the rebase and adjustments.

One more small change 😁.

Also, @juliushaertl, could you make a quick additional review?

src/components/SettingsForm.vue Outdated Show resolved Hide resolved
Signed-off-by: Marvin Öhlerking <marvin.oehlerking@capgemini.com>
@Loki-Afro
Copy link

@julien-nc any update on this?

@juliusknorr juliusknorr self-requested a review December 19, 2022 08:48
@yscialom
Copy link

yscialom commented Jan 8, 2023

Yet another user excited by this PR.
Any update? Available for tests in real-life environment if need be.

@chamabreu
Copy link

Hey, when will this be merged?

I need that grouping asap 😄

@julien-nc julien-nc self-requested a review January 11, 2023 15:22
@kronn
Copy link

kronn commented Jan 22, 2023

Where would the integration docs be located? I managed to integrate our software with Nextcloud using this plugin and would like to add my notes there. Basically: you need to pass the data for the provisioned user as part of the id_token.

@kronn
Copy link

kronn commented Feb 20, 2023

I used the current main branch and merged this PR. I was able to build a new version with krankerl and could run it without problems. I could integrate with my oauth-provider which can talk OIDC and I could pass groups along. I was happy.

Now I have a hosted nextcloud in which I cannot easily installed custom apps. I may get shell-access or some help with that, but I would prefer having a released version with this PR merged. I also want the other fixes that went into the main branch.

Is it possible to do a merge and a release of this? How can I help to see this done?

@alweber-cap
Copy link

pinging @juliushaertl for approve/merge

@juliusknorr
Copy link
Member

Tested and works. Thanks a lot for this awesome contribution.

@juliusknorr juliusknorr merged commit cde8fe5 into nextcloud:main Feb 21, 2023
@arnegns arnegns deleted the N21-266-provisioning-and-id-poc branch February 21, 2023 12:33
@kronn
Copy link

kronn commented Feb 21, 2023

Thank you for the merge, this means a lot to me, @juliushaertl.

I saw that the actions on main all passed successfully. 🎉
How is the process for releasing a new version to the app store? Should I open a github issue for it?

@juliusknorr
Copy link
Member

I'll check this with @julien-nc on if there are any other pending topics, but then I'd see no blocker for a release :)

@Zegorax
Copy link

Zegorax commented Feb 26, 2023

Hello everyone, thank you very much for this feature.

I implemented it on my side but I get the following error in the logs:

Error: foreach() argument must be of type array|object, null given at /var/www/html/apps/user_oidc/lib/Service/ProvisioningService.php#146

@Zegorax
Copy link

Zegorax commented Feb 26, 2023

Hello everyone, thank you very much for this feature.

I implemented it on my side but I get the following error in the logs:

Error: foreach() argument must be of type array|object, null given at /var/www/html/apps/user_oidc/lib/Service/ProvisioningService.php#146

For anyone else having this problem, check that you have the correct OIDC scopes requested. In my case it solved the problem.

@yscialom
Copy link

yscialom commented May 4, 2023

Hello, I install user_oidc via occ (script included in my nextcloud docker image):

occ user_oidc:provider Authelia \
    --clientid="(redacted)" \
    --clientsecret="(redacted)" \
    --discoveryuri="https://auth.(redacted)/.well-known/openid-configuration" \
    --scope="openid groups email profile" \
    --unique-uid=0 \
    --mapping-uid="preferred_username" \
    --no-interaction

I cannot find, neither in the doc, neither in occ user_oidc:provider --help neither in this PR's code how to "check" the "use group provisioning" option from occ.

Could you help @juliushaertl?

@arnegns
Copy link

arnegns commented May 4, 2023

@yscialom You definitley can set it via nextcloud config like this maybe also via occ. did you tried id?

"user_oidc": {
"allow_multiple_user_backends": "0",
"userinfo_bearer_validation": "false",
"selfencoded_bearer_validation": "true",
"types": "authentication",
"enabled": "yes",
"provider-1-mappingQuota": "",
"provider-1-extraClaims": "",
"provider-1-jwksCache": "",
"provider-1-jwksCacheTimestamp": "",
"provider-1-uniqueUid": "0",
"provider-1-checkBearer": "1",
"provider-1-bearerProvisioning": "1",
"provider-1-mappingUid": "sub",
"provider-1-mappingDisplayName": "name",
"provider-1-mappingEmail": "email",
"provider-1-mappingGroups": "groups",
"provider-1-groupProvisioning": "1",
"provider-1-providerBasedId": "1",
"id4me_enabled": "0"
},

@yscialom
Copy link

yscialom commented May 9, 2023

@arnegns
I managed, indeed, to set it through occ:

occ config:app:set user_oidc provider-1-groupProvisioning --value 1

I find it regrettable though to have to "guess" or "find" what my newly created provider name (here provider-1-groupProvisioning) is for occ config. It makes it harder that it could be.

@MohammedNoureldin
Copy link

MohammedNoureldin commented May 29, 2023

Hi, could anybody help me to map an existing group in NC to a group in IdP?

For example if I have a group in my IdP called Administrator, and I want this group to be mapped to the default group admin in NC. Is it somehow possible? Or at least be able to automatically give my group Administrator admin permissions?

I am already receiving the groups in the token and I see them provisioned to the user, but I am not sure about mapping admin to Administrator (for example).

Is it only doable using a custom mapper in IdP, or can it be solved in a simpler way?

@chamabreu
Copy link

AFAIK real mapping to existinging (eg admin) nextcloud groups is not possible for now.

I have this problem, too, that i need an extra admin account to manage my nextcloud, which is not provided via keycloak.

@MohammedNoureldin
Copy link

Thank you for you reply, @chamabreu.

I guess if we write a simple custom protocol mapper, which converts Administrator group to admin in the groups claim, can achieve what we want. Doesn't it?

The other option is simply implementing a field called admin IdP group(s), which is used to tell which group of IdP can be handled as admin.

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.