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

feat: add optional owner param to syncer methods #58

Conversation

elierotenberg
Copy link
Contributor

@elierotenberg elierotenberg commented May 17, 2024

Previous implementation always used this.orgName for the required owner field, however the expected owner can be a user name, and not an org name.
For example, the Casdoor web app always sets the owner to "admin".
This in turns make it impossible to retrieve syncers created through the web app using the SDK.

This addition fixes this issue by adding an optional owner parameter to the syncer methods.

Previous implementation always used this.orgName
for the required owner field, however the expected
owner can be a user name, and not an org name[1].
For example, the Casdoor web app always sets the
owner to "admin" [2]. This in turns make it
impossible to retrieve syncers created through the
web app using the SDK.

This addition fixes this issue by adding an optional
owner parameter to the syncer methods.

[1]: https://github.com/casdoor/casdoor/blob/master/object/syncer.go#L71
[2]: https://github.com/casdoor/casdoor/blob/master/web/src/SyncerListPage.js#L30
@casbin-bot
Copy link

@tangyang9464 @imp2002 please review

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@hsluoyz
Copy link
Member

hsluoyz commented May 17, 2024

@elierotenberg see the Go SDK: https://github.com/casdoor/casdoor-go-sdk/blob/master/casdoorsdk/syncer_test.go

See the syncer test case and it passes. The CURD works.

@hsluoyz hsluoyz closed this May 17, 2024
@elierotenberg
Copy link
Contributor Author

The Go SDK has the exact same problem.
I will also create a PR there.

@hsluoyz
Copy link
Member

hsluoyz commented May 17, 2024

@elierotenberg what's wrong here? Do you think the test case is wrong?

@elierotenberg
Copy link
Contributor Author

elierotenberg commented May 17, 2024

The test case isn't wrong, and there is not bug per se, rather that it is not currently possible to query the endpoint with an owner value that is not the current orgName.
There is nothing in the back-end implementation imposing that the owner is the current orgName, as illustrated by the Casdoor web app.

This is a debatable choice on the web app side as well however, the existence of a separate field organization in the Syncer model indicates that these are 2 separate, possibly distinct values.

The use case I am facing is:

  1. I create a syncer through the web UI (owner is admin, organization is built-in),
  2. I fetch the syncers through the SDKs (this.config.orgName is built-in) and an empty array is returned.

The proposed change is fully backwards compatible and simply allows the caller to explicitly specify the owner, defaulting to the previous behavior (this.config.orgName).

@hsluoyz
Copy link
Member

hsluoyz commented May 18, 2024

@elierotenberg you should modify orgName, we don't add new parameter. Initialize new client with different orgName

@casdoor casdoor deleted a comment from elierotenberg May 19, 2024
@casdoor casdoor locked and limited conversation to collaborators May 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants