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

set_oauth_host_name function contain code that will never execute. #137

Open
sqlldev opened this issue Jan 19, 2022 · 3 comments · May be fixed by #144
Open

set_oauth_host_name function contain code that will never execute. #137

sqlldev opened this issue Jan 19, 2022 · 3 comments · May be fixed by #144

Comments

@sqlldev
Copy link

sqlldev commented Jan 19, 2022

# Derive OAuth Base Path if not given

Correct me if I'm wrong, but this code will never execute.
I would like to use this functionality :(

It's not a blocker or any critical bug, because I can pass oauth_host_name to ApiClient constructor.
Just letting you know this code looks strange.

@geoffpdevsup
Copy link

I work in developer support at DocuSign, and as I recall our customers used to be confused by setting this up in demo, missing the step of setting the oauth host, and ending up with the oath request being sent to production. That is the why we raise an error now instead of setting a default value. I have posted a bug to cleanup the unexecuted code.

@geoffpdevsup
Copy link

This bug is filed as [DCM-7258] Unexecuted Code in python API client.

@deckar01
Copy link

deckar01 commented Apr 27, 2022

There is an internal method that is still expecting this empty call to implicitly define the oauth server.

https://github.com/docusign/docusign-esign-python-client/blob/26ec78e/docusign_esign/client/api_client.py#L839-L852

Silently defaulting to production is bad, but it can be fixed without discarding the convenience. The underlying problem with the original code was that the else branch unconditionally used the production server. It could require the base path to match production to make it safe and convenient. If the base path is not defined, it should error. If the base path doesn't have a corresponding auth server, it should error also.


A slightly related topic: The way oauth_host_name is required across ApiClient methods is inconsistent.

  • These methods require it to be passed as an argument every call:
    • request_jwt_user_token
    • request_jwt_application_token
  • This method requires it to be set before calling, doesn't accept an arg for it, and validates it is set:
    • get_user_info
  • This method assumes it is set without validation, doesn't accept an arg for it, and will try to make a post request to the host name None if it wasn't set:
    • generate_access_token
  • This is the method that will error when trying to use the implicit logic that is now unreachable:
    • get_authorization_uri

The first 2 methods that require the positional arg can't be fixed without a major version bump (or 2 depending up your deprecation policies). I'm not sure what the maintainers' stance will be on set-then-call, optional-kwarg, or required-arg, but I would recommend settling on one and make sure all the methods use that pattern in the next (few?) major release(s?). generate_access_token has a bug and should validate the auth server in a patch.

@deckar01 deckar01 linked a pull request Apr 27, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants