Add ca_certs argument for oauth and dropbox client #385

stanislau-arkhipenka · 2021-09-14T21:49:37Z

This change adds ca_certs str argument for oauth and dropbox client classes. It may be used with applications that packed into executable archives (like python par or py2exe). It also may be useful with some security related scenarios.

Checklist

General Contributing

Have you read the Code of Conduct and signed the CLA?

Is This a Code Change?

Non-code related change (markdown/git settings etc)
SDK Code Change
Example/Test Code Change

Validation

Does tox pass?
Do the tests pass? (no integration tests)

greg-db · 2021-09-15T15:17:31Z

Thanks for putting this together! I'll ask the team to review this, though I can't promise when that would be.

stanislau-arkhipenka · 2021-10-04T11:43:49Z

Hey @greg-db , thanks for reply. May be you have any updates?

greg-db · 2021-10-04T14:19:04Z

@stanislau-arkhipenka This is still open with the team, but I don't have an update on it.

codecov · 2021-10-04T16:33:15Z

Codecov Report

Merging #385 (f862c4b) into main (3fa08e5) will decrease coverage by 0.57%.
The diff coverage is 73.33%.

@@            Coverage Diff             @@
##             main     #385      +/-   ##
==========================================
- Coverage   64.25%   63.67%   -0.58%     
==========================================
  Files          30       30              
  Lines       51974    51982       +8     
  Branches     5780     5781       +1     
==========================================
- Hits        33394    33098     -296     
- Misses      18420    18849     +429     
+ Partials      160       35     -125

Flag	Coverage Δ
integration	`?`
unit	`63.67% <73.33%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
dropbox/session.py	`88.23% <63.63%> (-11.77%)`	⬇️
dropbox/dropbox_client.py	`38.43% <100.00%> (-34.36%)`	⬇️
dropbox/oauth.py	`55.04% <100.00%> (ø)`
dropbox/stone_serializers.py	`0.00% <0.00%> (-100.00%)`	⬇️
dropbox/exceptions.py	`61.22% <0.00%> (-16.33%)`	⬇️
dropbox/check.py	`77.77% <0.00%> (-8.34%)`	⬇️
dropbox/common.py	`72.44% <0.00%> (-5.52%)`	⬇️
dropbox/users.py	`62.93% <0.00%> (-4.08%)`	⬇️
dropbox/team_common.py	`72.99% <0.00%> (-3.65%)`	⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fa08e5...f862c4b. Read the comment docs.

rogebrd · 2021-10-04T20:17:01Z

Hello @stanislau-arkhipenka, I took at look at the PR and code wise, it looks good to me. I want to get a better understanding for your use case before we make the decision to merge this. Could you help me understand why this is needed?

april · 2021-10-04T22:04:46Z

dropbox/session.py

+    def __init__(self, *args, **kwargs):
+        self._ca_certs = kwargs.pop("ca_certs", None) or _TRUSTED_CERT_FILE
+        if not self._ca_certs:
+            raise FileNotFoundError("CA certificate not found")


This error is a bit misleading - it's not checking for the presence of a readable file (although maybe it should be?) - it's simply checking to see if ca_certs or _TRUSTED_CERT_FILE is set.

stanislau-arkhipenka · 2021-11-08T23:32:14Z

Hello @rogebrd ,
Sorry for quite a long delay.
There are two possible scenarios when this code may be useful:

In a secure environment with https reverse proxy (effectively corporate term for "man in the middle"). This diff will allows to use custom certificate issued by company.
(my case) For cases when developed application is packed into executable archive like par or py2exe (issues mentioned here and here).

CLAassistant · 2022-04-16T21:47:39Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 3 committers have signed the CLA.

✅ stanislau-arkhipenka
❌ Stanislau Arkhipenka
❌ Brent1LT

Stanislau Arkhipenka seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

stanislau-arkhipenka · 2022-04-20T09:13:40Z

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.
Stanislau Arkhipenka seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Signed it

stanislau-arkhipenka · 2022-06-10T14:08:08Z

Hey guys, any updates? I've signed CLA. Do you need anything else on my side?
Thanks!

greg-db · 2022-06-13T14:22:17Z

Thanks for checking in. This is open with the team, but I don't have an update or timeline on it right now.

Brent1LT · 2022-06-21T16:27:48Z

Everything looks good to me but it looks like there are some integration tests that are failing before it can get merged in.

New error better represents actual problem (CA not set) And also compatible with python2.7

stanislau-arkhipenka · 2022-06-29T12:03:11Z

Everything looks good to me but it looks like there are some integration tests that are failing before it can get merged in.

Hey @Brent1LT , thanks for reply!
Indeed there was a problem with FileNotFoundError (it doesn't supported in python2.7) - FIXED.
With regards to integration tests: I believe it is not related to this particular PR, as it seems persists across other open PRs.
Based on error message my best guess is that some environmental variables on CI side are not populated correctly. I'm not sure, but it may be related to rename of envs from DROPBOX_* to SCOPED_* (btw same problem exists if you try to run tox -e test_integration )

stanislau-arkhipenka · 2022-06-29T22:40:37Z

Yep,
credentials for integration tests are not populated.
If you open failed Integration test -> Run Integration Tests -> Run pytest test/integration/test_dropbox.py you will see that credentials are not set. Here is an example https://paste.pics/99b08a30383cb84552645493745caa22

stanislau-arkhipenka · 2022-06-29T22:44:56Z

@rogebrd , @april , @Brent1LT
How do you think can this PR be merged to upstream?
The Integration tests are failing because of missed environmental variables, and it doesn't relate to PR

sderickson · 2022-06-30T17:32:25Z

@rogebrd , @april , @Brent1LT How do you think can this PR be merged to upstream? The Integration tests are failing because of missed environmental variables, and it doesn't relate to PR

Those are because only Dropboxers can run those tests with those sensitive variables. Hence why they aren't required. Moving forward with merging - thanks for doing this!

dennissiemensma · 2022-07-27T21:06:20Z

I've added a minor follow-up for this PR in #440 to prevent pickle from breaking.

Add ca_certs argument for oauth and dropbox client

22a45aa

april reviewed Oct 4, 2021

View reviewed changes

Merge branch 'main' into add_ca_certs_arg

45bcefb

Replace FileNotFoundError with AttributeError

f862c4b

New error better represents actual problem (CA not set) And also compatible with python2.7

sderickson approved these changes Jun 30, 2022

View reviewed changes

sderickson merged commit f3f8729 into dropbox:main Jun 30, 2022

dennissiemensma mentioned this pull request Jul 27, 2022

Added _ca_certs property to _SSLAdapter to properly support pickling (broke in SDK v11.33) #440

Merged

7 tasks

sagarvora mentioned this pull request Apr 5, 2023

Fix invalid specifier in stone requirement #456

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ca_certs argument for oauth and dropbox client #385

Add ca_certs argument for oauth and dropbox client #385

stanislau-arkhipenka commented Sep 14, 2021 •

edited

Loading

greg-db commented Sep 15, 2021

stanislau-arkhipenka commented Oct 4, 2021

greg-db commented Oct 4, 2021

codecov bot commented Oct 4, 2021 •

edited

Loading

rogebrd commented Oct 4, 2021

april Oct 4, 2021

stanislau-arkhipenka commented Nov 8, 2021

CLAassistant commented Apr 16, 2022 •

edited

Loading

stanislau-arkhipenka commented Apr 20, 2022

stanislau-arkhipenka commented Jun 10, 2022

greg-db commented Jun 13, 2022

Brent1LT commented Jun 21, 2022

stanislau-arkhipenka commented Jun 29, 2022

stanislau-arkhipenka commented Jun 29, 2022

stanislau-arkhipenka commented Jun 29, 2022

sderickson commented Jun 30, 2022

dennissiemensma commented Jul 27, 2022

Add ca_certs argument for oauth and dropbox client #385

Add ca_certs argument for oauth and dropbox client #385

Conversation

stanislau-arkhipenka commented Sep 14, 2021 • edited Loading

Checklist

greg-db commented Sep 15, 2021

stanislau-arkhipenka commented Oct 4, 2021

greg-db commented Oct 4, 2021

codecov bot commented Oct 4, 2021 • edited Loading

Codecov Report

rogebrd commented Oct 4, 2021

april Oct 4, 2021

Choose a reason for hiding this comment

stanislau-arkhipenka commented Nov 8, 2021

CLAassistant commented Apr 16, 2022 • edited Loading

stanislau-arkhipenka commented Apr 20, 2022

stanislau-arkhipenka commented Jun 10, 2022

greg-db commented Jun 13, 2022

Brent1LT commented Jun 21, 2022

stanislau-arkhipenka commented Jun 29, 2022

stanislau-arkhipenka commented Jun 29, 2022

stanislau-arkhipenka commented Jun 29, 2022

sderickson commented Jun 30, 2022

dennissiemensma commented Jul 27, 2022

stanislau-arkhipenka commented Sep 14, 2021 •

edited

Loading

codecov bot commented Oct 4, 2021 •

edited

Loading

CLAassistant commented Apr 16, 2022 •

edited

Loading