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

prevent empty json object {} from passing to http GET body #11

Merged
merged 5 commits into from
Sep 11, 2023

Conversation

imagenetmit
Copy link
Contributor

update connectwise_endpoint.py to remove json=data from requests.request() args when data is empty. Otherwise, it inserts an empty JSON object {} into the http GET request body. The CW api doesn't like that, so it returns a 403. I thought it might be a python specific issue for me, but I can reproduce the error in Postman by adding {} to the GET request.

@Yoshify
Copy link
Contributor

Yoshify commented Sep 11, 2023

Thanks for the PR!

Could I ask what endpoint you're trying to GET and getting a 403 from? I can't replicate this issue from within the library or in postman (at least with company/companies and service/tickets). I wonder if the issue is endpoint specific?

@Yoshify Yoshify added the question Further information is requested label Sep 11, 2023
@imagenetmit
Copy link
Contributor Author

https://na.myconnectwise.net/v2022_2/apis/3.0/company/companies

I'm wondering if it might be specific to the NA cloud as opposed to the AU cloud. You guys are in Australia, right?

@imagenetmit
Copy link
Contributor Author

Scratch that, I just tested against https://au.myconnectwise.net/v2022_2/apis/3.0/company/companies, and it does the same thing.
Body: none - sails through
Body: {} - 403

@Yoshify
Copy link
Contributor

Yoshify commented Sep 11, 2023

Yep - but we also use on-prem Manage. I wonder if this is a difference in how cloud handles requests? Unfortunately I don't have a cloud environment readily on hand to test with.

That's strange behavior if so. Regardless - this is a non-breaking change and I'm happy to merge this in order to properly support cloud environ's.

Thanks for your help, and for the PR :)

@Yoshify Yoshify merged commit 210527c into HealthITAU:main Sep 11, 2023
@imagenetmit
Copy link
Contributor Author

Thank you! That was my first PR, ever. If you need any testing done against CW cloud environs, I'm happy to help.

@Yoshify Yoshify added the bug Something isn't working label Sep 11, 2023
@EMoonArchiTech
Copy link
Contributor

Sorry to bring this one back up but would this warrant a new minor release provided it passes any tests you have set up? We have cloud as well and I'm just getting 403's on the release version.

@Yoshify
Copy link
Contributor

Yoshify commented Sep 13, 2023

Yep! I'll be pushing some changes tomorrow or later tonight to come in alongside it. Changes will also pertain to #12
If you're in a rush please feel free to fork the repo and install with Pip locally :)

@Yoshify
Copy link
Contributor

Yoshify commented Sep 14, 2023

@imagenetmit @EMoonArchiTech 0.4.6 has been released, including this PR. Could I please get you guys to test and confirm that you no longer get 403's?

@EMoonArchiTech
Copy link
Contributor

Working perfect for me! :)

@imagenetmit
Copy link
Contributor Author

@Yoshify works beautifully now!

@Yoshify
Copy link
Contributor

Yoshify commented Sep 15, 2023

Very glad to hear that - thank you both for testing and confirming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants