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

[FR] Groups claim #320

Open
Joker9944 opened this issue Sep 3, 2024 · 6 comments
Open

[FR] Groups claim #320

Joker9944 opened this issue Sep 3, 2024 · 6 comments

Comments

@Joker9944
Copy link

Hello there!

Short question. After reading trough the auto-provision config options I noticed that there is only an option to hardcode groups to be assigned to all new users and no option to set a groups claim

Is there a reason that there is no option to configure a groups claim?

This would enable me to handle group membership for existing and new users in IDM.

If this is a desired feature I would be happy to volunteer my time. Just wanted to make sure this is actually feasible and desirable.

@DeepDiver1975
Copy link
Member

Hey. Thanks for offering your support!
Contributions are always welcome!

I am not aware of any standard claims we could use. What IDP are you using and could you please post an example (jwt access token or ID token or user endpoint response).

Thanks

@Joker9944
Copy link
Author

There isn't a standard claim sadly but there are some examples out there.

I personally use Kanidm which similar to Authilia has a groups scope and claim. Additionally it supports custom claims.

In essence the requirement would be being able to specify an optional groups-claim which has all the group names a user should be added to as a json array.

Additionally there can be groups which do not map to existing ownCloud groups since the IDM exposes all groups a user is in. For example a user cloud be in a group which allows access to a completely different application and has nothing to do with ownCloud. So I think the approach would be to only add users to groups which already exist.

I will make a PR then.

@DeepDiver1975
Copy link
Member

Additionally there can be groups which do not map to existing ownCloud groups since the IDM exposes all groups a user is in. For example a user cloud be in a group which allows access to a completely different application and has nothing to do with ownCloud. So I think the approach would be to only add users to groups which already exist.

the interesting part here will be the mapping of the group identifiers ...

I will make a PR then.

Cool - let me know if you have any questions!

@Joker9944
Copy link
Author

I have written the code and some unit tests but struggling to execute those tests.

After some digging I found that the phpunit bin should live at "$(PWD)/../../lib/composer/bin/phpunit" and that a file at /../../../tests/bootstrap.php is required for running tests.

Is there some special project setup that is needed to run unit tests?

Sorry if this is a dumb question, I do not have much experience with php.

@Joker9944
Copy link
Author

Also I have two implementation questions.

An additional scope will be needed next to the ones already required. Since this feature should be optional and there isn't a official standard for a scope like groups I do not want to add any arbitrary claim by default. So I would propose adding an additional config field groups-scope where the requested scope can be configured. This scope can then be added to the default ones taken from config. Alternatively this can be solved in documentation by pointing out that the scope should be specified in 'openid-connect' => [ 'scopes' => ['openid', 'profile', 'email', 'groups'] ].

Adding a user to groups is only done in createUser but not updated in updateAccountInfo currently. I would love to add this functionally but wanted to first ask if this has been left out for a reason first.

@DeepDiver1975
Copy link
Member

Is there some special project setup that is needed to run unit tests?

testing owncloud apps is described in here - https://doc.owncloud.com/server/next/developer_manual/testing/unit-testing.html

An additional scope will be needed next to the ones already required. Since this feature should be optional and there isn't a official standard for a scope like groups I do not want to add any arbitrary claim by default. So I would propose adding an additional config field groups-scope where the requested scope can be configured. This scope can then be added to the default ones taken from config. Alternatively this can be solved in documentation by pointing out that the scope should be specified in 'openid-connect' => [ 'scopes' => ['openid', 'profile', 'email', 'groups'] ].

The first proposed approach sounds god for me. THX

Adding a user to groups is only done in createUser but not updated in updateAccountInfo currently. I would love to add this functionally but wanted to first ask if this has been left out for a reason first.

most probably something which got forgotten - feel free to fix this as well. preferably in a separate pull request.

THX

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

No branches or pull requests

2 participants