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

SNOW-761004 Added URL Validator and URL escaping of strings #1480

Merged
merged 17 commits into from Mar 23, 2023

Conversation

sfc-gh-hchaturvedi
Copy link
Collaborator

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-761004

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am modifying authorization mechanisms
    • I am adding new credentials
    • I am modifying OCSP code
    • I am adding a new dependency
  3. Please describe how your code solves the related issue.

    Currently Python connector does not ensure the validity of the SSO URL and just opens it in the browser. The connector should ensure that the URL is a valid HTTP / HTTPS URL before passing it to the Browser. This PR also adds support for URL encoding strings. This will be used later for encoding Base64 OCSP requests before sending them to the OCSP Responder.

@github-actions
Copy link

github-actions bot commented Mar 17, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@sfc-gh-hchaturvedi
Copy link
Collaborator Author

I have read the CLA Document and I hereby sign the CLA

src/snowflake/connector/auth/webbrowser.py Outdated Show resolved Hide resolved
src/snowflake/connector/url_util.py Outdated Show resolved Hide resolved
test/unit/test_url_util.py Outdated Show resolved Hide resolved
src/snowflake/connector/url_util.py Outdated Show resolved Hide resolved
src/snowflake/connector/url_util.py Outdated Show resolved Hide resolved
src/snowflake/connector/url_util.py Outdated Show resolved Hide resolved
src/snowflake/connector/url_util.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 17, 2023

Codecov Report

Merging #1480 (e37b53c) into main (4b1d474) will increase coverage by 0.09%.
The diff coverage is 100.00%.

❗ Current head e37b53c differs from pull request most recent head 3f1c345. Consider uploading reports for the commit 3f1c345 to get more accurate results

@@            Coverage Diff             @@
##             main    #1480      +/-   ##
==========================================
+ Coverage   81.90%   81.99%   +0.09%     
==========================================
  Files          60       61       +1     
  Lines        8598     8616      +18     
  Branches     1271     1273       +2     
==========================================
+ Hits         7042     7065      +23     
+ Misses       1223     1218       -5     
  Partials      333      333              
Impacted Files Coverage Δ
src/snowflake/connector/snow_logging.py 56.75% <ø> (ø)
src/snowflake/connector/auth/webbrowser.py 71.27% <100.00%> (+0.78%) ⬆️
src/snowflake/connector/url_util.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sfc-gh-hchaturvedi sfc-gh-hchaturvedi marked this pull request as ready for review March 17, 2023 20:43
@sfc-gh-hchaturvedi sfc-gh-hchaturvedi enabled auto-merge (squash) March 17, 2023 20:43
@sfc-gh-mkeller
Copy link
Collaborator

Please rebase your PR @sfc-gh-hchaturvedi we have updated the license header years to include 2023

@sfc-gh-kdama
Copy link
Collaborator

@sfc-gh-mkeller @sfc-gh-hchaturvedi is this good to merge?

Returns:
URL encoded string
"""
if target is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

When does target evaluate to None?
Why do we need this wrapper if it's just a one-liner calling urllib.parse.quote_plus?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will be part of the OCSP Code base where we need to make sure all the OCSP Request besides being baset64 encoded are also URL Encoded. The OCSP Code base depends a lot on what is received int the certificate from the Certificate authority. If in case the information is bad and the certificate request generated turns out to be None this case should catch that. This utility function also gives us the ability to add more such cases which I am sure there will be.

DESCRIPTION.md Outdated Show resolved Hide resolved
@sfc-gh-kdama
Copy link
Collaborator

@sfc-gh-hchaturvedi @sfc-gh-stan checking status on this as the release is waiting on this. Looks the merge gates are failing on this.

DESCRIPTION.md Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
test/unit/test_url_util.py Outdated Show resolved Hide resolved
#
import pytest

from snowflake.connector.url_util import is_valid_url, url_encode_str
Copy link
Collaborator

Choose a reason for hiding this comment

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

we also need to ignore the import error otherwise other driver just doesn't have this module or we vendor the method in the tests folder

Suggested change
from snowflake.connector.url_util import is_valid_url, url_encode_str
try:
from snowflake.connector.url_util import is_valid_url, url_encode_str
except ImportError:
is_valid_url = None
url_encode_str = None

@sfc-gh-mkeller sfc-gh-mkeller force-pushed the hchaturvedi_snow_761004_add_url_validator branch from e37b53c to d5ed78b Compare March 22, 2023 23:38
@sfc-gh-mkeller sfc-gh-mkeller force-pushed the hchaturvedi_snow_761004_add_url_validator branch from d5ed78b to ef92f28 Compare March 22, 2023 23:40
Copy link
Collaborator

@sfc-gh-mkeller sfc-gh-mkeller left a comment

Choose a reason for hiding this comment

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

🚢 🚢

@sfc-gh-hchaturvedi sfc-gh-hchaturvedi merged commit 1cdbd3b into main Mar 23, 2023
35 checks passed
@sfc-gh-hchaturvedi sfc-gh-hchaturvedi deleted the hchaturvedi_snow_761004_add_url_validator branch March 23, 2023 00:16
@github-actions github-actions bot locked and limited conversation to collaborators Mar 23, 2023
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.

None yet

5 participants