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

add memberOf to /users endpoint and members to /groups endpoint #3925

Merged
merged 9 commits into from
Jun 9, 2022

Conversation

dragonchaser
Copy link
Contributor

@dragonchaser dragonchaser commented Jun 7, 2022

This PR adds a members slice to the /groups endpoint and
a members slice to the /users endpoint.

refs #3837

@update-docs
Copy link

update-docs bot commented Jun 7, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@dragonchaser dragonchaser force-pushed the improve-graph branch 2 times, most recently from 8f10bc4 to adbf879 Compare June 7, 2022 13:43
@ownclouders
Copy link
Contributor

ownclouders commented Jun 7, 2022

💥 Acceptance test Core-API-Tests-ocis-storage-8 failed. Further test are cancelled...

@dragonchaser dragonchaser changed the title [WIP] add groups to /users endpoint add memberOf to /users endpoint and members to /groups endpoint Jun 8, 2022
@dragonchaser dragonchaser marked this pull request as ready for review June 8, 2022 19:13
@dragonchaser dragonchaser requested review from rhafer and butonic June 8, 2022 19:13
Copy link
Contributor

@rhafer rhafer left a comment

Choose a reason for hiding this comment

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

Just a few minor things. Looks good to me otherwise.

extensions/graph/pkg/identity/ldap.go Outdated Show resolved Hide resolved
extensions/graph/pkg/identity/ldap.go Outdated Show resolved Hide resolved
@dragonchaser dragonchaser requested a review from rhafer June 9, 2022 11:46
dragonchaser and others added 9 commits June 9, 2022 14:35
Signed-off-by: Christian Richter <[email protected]>
Signed-off-by: Christian Richter <[email protected]>
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: Christian Richter <[email protected]>
Signed-off-by: Christian Richter <[email protected]>
Signed-off-by: Christian Richter <[email protected]>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 10 Code Smells

39.1% 39.1% Coverage
16.6% 16.6% Duplication

@dragonchaser dragonchaser merged commit bea9f2c into master Jun 9, 2022
@delete-merged-branch delete-merged-branch bot deleted the improve-graph branch June 9, 2022 13:21
ownclouders pushed a commit that referenced this pull request Jun 9, 2022
Merge: 782dd08 c4f7a36
Author: Christian Richter <[email protected]>
Date:   Thu Jun 9 15:21:14 2022 +0200

    Merge pull request #3925 from owncloud/improve-graph

    add memberOf to /users endpoint and members to /groups endpoint
@phil-davis
Copy link
Contributor

phil-davis commented Jun 13, 2022

@dragonchaser I have built an ocis with the latest master and run it:

make clean
make generate
cd ocis
make build
cd bin
PROXY_ENABLE_BASIC_AUTH=true OCIS_INSECURE=true ./ocis server

But my attempts to use the new member API queries are not successful:

$ curl -k 'https://localhost:9200/graph/v1.0/users' -u admin:admin
{"value":[{"displayName":"Admin","id":"473ecfbe-1137-4793-8a8e-14cc2d1e5b1e","mail":"[email protected]","onPremisesSamAccountName":"admin"},{"displayName":"Phil Davis","id":"bad24955-8c2a-4bf6-a13e-d14ec2a587d1","mail":"[email protected]","onPremisesSamAccountName":"phil"},{"displayName":"Mr Test","id":"654f7536-c1f5-4e06-bbc5-d1630e452956","mail":"[email protected]","onPremisesSamAccountName":"test123"}]}
$ curl -k 'https://localhost:9200/graph/v1.0/groups' -u admin:admin
{"value":[{"displayName":"g42","id":"31572dac-a4b5-419b-b2e5-3390e6144386"},{"displayName":"z999","id":"26d47151-ecb2-4d82-a82b-01444998068d"}]}
$ curl -k 'https://localhost:9200/graph/v1.0/groups/31572dac-a4b5-419b-b2e5-3390e6144386/members' -u admin:admin
[{"displayName":"Phil Davis","id":"bad24955-8c2a-4bf6-a13e-d14ec2a587d1","mail":"[email protected]","onPremisesSamAccountName":"phil"}]

phil (bad24955-8c2a-4bf6-a13e-d14ec2a587d1) is in group g42 (31572dac-a4b5-419b-b2e5-3390e6144386)

But when I try to get the group(s) along with the members:

$ curl -k 'https://localhost:9200/graph/v1.0/groups?$expand=members' -u admin:admin
{"value":[{"displayName":"g42","id":"31572dac-a4b5-419b-b2e5-3390e6144386"},{"displayName":"z999","id":"26d47151-ecb2-4d82-a82b-01444998068d"}]}

I just get a response with the group display names.

Or query a single group:

$ curl -k 'https://localhost:9200/graph/v1.0/groups/31572dac-a4b5-419b-b2e5-3390e6144386/?$expand=members' -u admin:admin
{"displayName":"g42","id":"31572dac-a4b5-419b-b2e5-3390e6144386"}

I just get a response with the group display name.

Am I missing something?

@dragonchaser
Copy link
Contributor Author

@phil-davis could not reproduce

$> curl -k $ curl -k 'https://localhost:9200/graph/v1.0/groups/509a9dcd-bb37-4f4f-a01a-19dca27d9cfa?$expand=members' -u admin:admin|jq
{
  "displayName": "users",
  "id": "509a9dcd-bb37-4f4f-a01a-19dca27d9cfa",
  "members": [
    {
      "displayName": "Albert Einstein",
      "id": "4c510ada-c86b-4815-8820-42cdf82c3d51",
      "mail": "[email protected]",
      "onPremisesSamAccountName": "einstein"
    },
    {
      "displayName": "Marie Skłodowska Curie",
      "id": "f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c",
      "mail": "[email protected]",
      "onPremisesSamAccountName": "marie"
    },
    {
      "displayName": "Richard Phillips Feynman",
      "id": "932b4540-8d16-481e-8ef4-588e4b6b151c",
      "mail": "[email protected]",
      "onPremisesSamAccountName": "richard"
    },
    {
      "displayName": "Maurice Moss",
      "id": "058bff95-6708-4fe5-91e4-9ea3d377588b",
      "mail": "[email protected]",
      "onPremisesSamAccountName": "moss"
    },
    {
      "displayName": "Admin",
      "id": "some-admin-user-id-0000-000000000000",
      "mail": "[email protected]",
      "onPremisesSamAccountName": "admin"
    }
  ]
}

Can you run ocis version and paste the output?

@phil-davis
Copy link
Contributor

@dragonchaser I pulled current master and rebuilt my oCIS:

$ ./ocis version
Version: 2.0.0-beta.3+ab221d2b2
Compiled: 2022-06-14 00:00:00 +0000 UTC

No running services found.

And I still have the same problem.

@phil-davis
Copy link
Contributor

I added another user to group "g42". Now all the expand=members and expand=memberOf queries are working.

curl -k --header "Content-Type: application/json" \
        --request POST  --data \
	'{ "@odata.id": "https://localhost:9200/graph/v1.0/users/acc15432-9dc4-424c-91d7-6867572a244c" }' \
	'https://localhost:9200/graph/v1.0/groups/31572dac-a4b5-419b-b2e5-3390e6144386/members/$ref' -u admin:admin

Probably I already had user phil in group g42 before this new feature was added to oCIS. "something" seems to have "rebuilt" the user<=>group relationship information "somewhere" and it works now.

@dragonchaser
Copy link
Contributor Author

Confusing, there have been no changes to these structures, the change just adds LDAP queries to the code.

@phil-davis
Copy link
Contributor

Confusing, there have been no changes to these structures, the change just adds LDAP queries to the code.

It WFM now, so we can't do anything more really to investigate.

@micbar micbar mentioned this pull request Jun 22, 2022
40 tasks
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.

5 participants