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

Change md5 to hash and hash_algorithm, fix incompatibility #1367

Merged
merged 16 commits into from Nov 24, 2023

Conversation

Wh1isper
Copy link
Contributor

Resolves #1366 mwouts/jupytext#1165 ploomber/ploomber#1144

Sorry for the breaking changes to the API. This should allow downstream optional support for md5.

@Wh1isper
Copy link
Contributor Author

@LoicGrobol @fcollonval @marr75

Please review if this fixes most of the incompatibilities.

@blink1073
Copy link
Collaborator

In hindsight based on @bollwyvl's comment, I agree we should move to sha256, which is FIPS-compliant.

@Wh1isper
Copy link
Contributor Author

@blink1073 Okay, I think I can change all the md5 to sha256. Do you want me to open another pr?

@blink1073
Copy link
Collaborator

We can re-use this one.

@Wh1isper Wh1isper changed the title Making MD5 optional for ContentManager Change md5 to sha256 and make it optional for ContentManager Nov 23, 2023
@blink1073
Copy link
Collaborator

Thanks! I think we should add jupytext as one of our downstream tests. Would you like to try that? If not I can do it in a follow up PR.

@blink1073 blink1073 added the bug label Nov 23, 2023
@Wh1isper
Copy link
Contributor Author

@blink1073

I think we should add jupytext as one of our downstream tests. Would you like to try that?

Yeah, I'll give it a shot.

uses: jupyterlab/maintainer-tools/.github/actions/downstream-test@v1
with:
package_name: jupytext
test_command: pip install pytest-jupyter[server] gitpython pre-commit && python -m ipykernel install --name jupytext-dev --user && pytest -vv -raXxs -W default --durations 10 --color=yes --ignore=tests/test_doc_files_are_notebooks.py --ignore=tests/test_changelog.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test_doc_files_are_notebooks.py and test_changelog.py need docs floder, but not in the distribution.

@Wh1isper
Copy link
Contributor Author

@fcollonval
Copy link
Member

Thanks @Wh1isper to work quickly on a fix an @blink1073 to review the code.

I started something yesterday but did not get the chance to finish. Basically I was proposing to return two attributes hash and hash_algorithm to have something generic as security wise what is now secure may not be in the future.

My code was ready I was only looking at how to test this without explicitly adding a dependency like jupytext that may evolve in the future.

If you give 3h (it's still very early for me 😅) I can contribute to this one.

@Wh1isper
Copy link
Contributor Author

Wh1isper commented Nov 23, 2023

@fcollonval 😄 Good morning, I've just finished lunch being in GMT+8.

Returning hash and hash_algorithm is a good idea, it requires us to have a HashManager and then choose a default hash_algorithm. I wonder if it would be possible to allow the client to choose the hash_algorithm, which might be useful (or maybe not).

@Wh1isper
Copy link
Contributor Author

I'd like to try to add hash and hash_algorithm, please review this PR later.

Comment on lines 155 to 156
if cm.support_hash:
kwargs["require_hash"] = require_hash
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the ContentsManager does not declare itself to support hash(by setting support_hash to True), handler will never pass require_hash argument to the ContentsManager.get.

I don't want to use hash for the argument as it's a built-in function.

@Wh1isper Wh1isper changed the title Change md5 to sha256 and make it optional for ContentManager Change md5 to hash and hash_algorithm, make compatible with ContentManager API. Nov 23, 2023
@Wh1isper Wh1isper changed the title Change md5 to hash and hash_algorithm, make compatible with ContentManager API. Change md5 to hash and hash_algorithm, fix incompatibility Nov 23, 2023
@Wh1isper
Copy link
Contributor Author

Ready for review.

@fcollonval
Copy link
Member

@Wh1isper I pushed in fcollonval#1 a bunch of improvements. Feel free to cherry-pick them.

@fcollonval
Copy link
Member

I would also strongly suggest removing the downstream test against Jupytext because it is not an official Jupyter subproject and we don't have a say on it.

To check for no regression, my PR contains an appropriate ContentsManager mock up.

@blink1073
Copy link
Collaborator

I think it is fair to test against a handful of popular community projects. I've seen that pattern used by other projects like mypy and ruff.

@davidbrochart
Copy link
Contributor

The file hash is actually something we could take advantage of in collaborative editing. We currently compare timestamps to check if the server was the one who saved the file last, and if not then we get the new content from disk.
Having a file hash would be more reliable. Thanks for the idea and for you work @Wh1isper!

Wh1isper and others added 4 commits November 24, 2023 00:33
@Wh1isper
Copy link
Contributor Author

@fcollonval Thank you. I've learned a lot from that!

To pass the typing test, I added a number of # type: ignore.

@Wh1isper
Copy link
Contributor Author

😀 I'd like to ask you to review again.

Thank you all!

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks a lot for quickly working on this @Wh1isper

@blink1073 blink1073 merged commit ecd5b1f into jupyter-server:main Nov 24, 2023
36 checks passed
@blink1073
Copy link
Collaborator

Thanks again @Wh1isper! I'll make a new release early next week.

@Wh1isper
Copy link
Contributor Author

My pleasure. Thank you all :D

sigmarkarl added a commit to spotinst/jupyter_server that referenced this pull request Dec 13, 2023
* ContentsHandler return 404 rather than raise exc (jupyter-server#1357)

* Add more typings (jupyter-server#1356)

* Publish 2.10.1

SHA256 hashes:

jupyter_server-2.10.1-py3-none-any.whl: 20519e355d951fc5e1b6ac5952854fe7620d0cfb56588fa4efe362a758977ed3

jupyter_server-2.10.1.tar.gz: e6da2657a954a7879eed28cc08e0817b01ffd81d7eab8634660397b55f926472

* Bump to 2.11.0.dev0

* typo: ServerApp (jupyter-server#1361)

* Support get file(notebook) md5 (jupyter-server#1363)

* Update ruff and typings (jupyter-server#1365)

* Update api docs with md5 param (jupyter-server#1364)

* Publish 2.11.0

SHA256 hashes:

jupyter_server-2.11.0-py3-none-any.whl: c9bd6e6d71dc5a2a25df167dc323422997f14682b008bfecb5d7920a55020ea7

jupyter_server-2.11.0.tar.gz: 78c97ec8049f9062f0151725bc8a1364dfed716646a66819095e0e8a24793eba

* Bump to 2.12.0.dev0

* Change md5 to hash and hash_algorithm, fix incompatibility (jupyter-server#1367)

Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>

* avoid unhandled error on some invalid paths (jupyter-server#1369)

* Publish 2.11.1

SHA256 hashes:

jupyter_server-2.11.1-py3-none-any.whl: 4b3a16e3ed16fd202588890f10b8ca589bd3e29405d128beb95935f059441373

jupyter_server-2.11.1.tar.gz: fe80bab96493acf5f7d6cd9a1575af8fbd253dc2591aa4d015131a1e03b5799a

* Bump to 2.12.0.dev0

* Merge pull request from GHSA-h56g-gq9v-vc8r

Co-authored-by: Steven Silvester <steven.silvester@ieee.org>

* Publish 2.11.2

SHA256 hashes:

jupyter_server-2.11.2-py3-none-any.whl: 0c548151b54bcb516ca466ec628f7f021545be137d01b5467877e87f6fff4374

jupyter_server-2.11.2.tar.gz: 0c99f9367b0f24141e527544522430176613f9249849be80504c6d2b955004bb

* Bump to 2.12.0.dev0

* chore: update pre-commit hooks (jupyter-server#1370)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Steven Silvester <steven.silvester@ieee.org>

* Update for tornado 6.4 (jupyter-server#1372)

* Support async Authorizers (jupyter-server#1373)

* Publish 2.12.0

SHA256 hashes:

jupyter_server-2.12.0-py3-none-any.whl: 3482912efa4387bb1edc23ba60531796aff3b6d6a6e93a5810f5719e2bdb48b7

jupyter_server-2.12.0.tar.gz: 9fa74ed3bb931cf33f42b3d9046e2788328ec9e6dcc59d48aa3e0910a491e3e4

* Bump to 2.13.0.dev0

* log extension import time at debug level unless it's actually slow (jupyter-server#1375)

* Add support for async Authorizers (part 2) (jupyter-server#1374)

* Publish 2.12.1

SHA256 hashes:

jupyter_server-2.12.1-py3-none-any.whl: fd030dd7be1ca572e4598203f718df6630c12bd28a599d7f1791c4d7938e1010

jupyter_server-2.12.1.tar.gz: dc77b7dcc5fc0547acba2b2844f01798008667201eea27c6319ff9257d700a6d

---------

Co-authored-by: Sam Bloomquist <bloomquist.sam@gmail.com>
Co-authored-by: Steven Silvester <steven.silvester@ieee.org>
Co-authored-by: blink1073 <blink1073@users.noreply.github.com>
Co-authored-by: IITII <ccmejx@gmail.com>
Co-authored-by: Zhongsheng Ji <9573586@qq.com>
Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>
Co-authored-by: Min RK <benjaminrk@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Zachary Sailer <zsailer@apple.com>
Co-authored-by: Zsailer <Zsailer@users.noreply.github.com>
sigmarkarl added a commit to spotinst/jupyter_server that referenced this pull request Jan 15, 2024
* Create CODEOWNERS

* ContentsHandler return 404 rather than raise exc (jupyter-server#1357)

* Add more typings (jupyter-server#1356)

* Publish 2.10.1

SHA256 hashes:

jupyter_server-2.10.1-py3-none-any.whl: 20519e355d951fc5e1b6ac5952854fe7620d0cfb56588fa4efe362a758977ed3

jupyter_server-2.10.1.tar.gz: e6da2657a954a7879eed28cc08e0817b01ffd81d7eab8634660397b55f926472

* Bump to 2.11.0.dev0

* typo: ServerApp (jupyter-server#1361)

* Support get file(notebook) md5 (jupyter-server#1363)

* Update ruff and typings (jupyter-server#1365)

* Update api docs with md5 param (jupyter-server#1364)

* Publish 2.11.0

SHA256 hashes:

jupyter_server-2.11.0-py3-none-any.whl: c9bd6e6d71dc5a2a25df167dc323422997f14682b008bfecb5d7920a55020ea7

jupyter_server-2.11.0.tar.gz: 78c97ec8049f9062f0151725bc8a1364dfed716646a66819095e0e8a24793eba

* Bump to 2.12.0.dev0

* Change md5 to hash and hash_algorithm, fix incompatibility (jupyter-server#1367)

Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>

* avoid unhandled error on some invalid paths (jupyter-server#1369)

* Publish 2.11.1

SHA256 hashes:

jupyter_server-2.11.1-py3-none-any.whl: 4b3a16e3ed16fd202588890f10b8ca589bd3e29405d128beb95935f059441373

jupyter_server-2.11.1.tar.gz: fe80bab96493acf5f7d6cd9a1575af8fbd253dc2591aa4d015131a1e03b5799a

* Bump to 2.12.0.dev0

* Merge pull request from GHSA-h56g-gq9v-vc8r

Co-authored-by: Steven Silvester <steven.silvester@ieee.org>

* Publish 2.11.2

SHA256 hashes:

jupyter_server-2.11.2-py3-none-any.whl: 0c548151b54bcb516ca466ec628f7f021545be137d01b5467877e87f6fff4374

jupyter_server-2.11.2.tar.gz: 0c99f9367b0f24141e527544522430176613f9249849be80504c6d2b955004bb

* Bump to 2.12.0.dev0

* chore: update pre-commit hooks (jupyter-server#1370)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Steven Silvester <steven.silvester@ieee.org>

* Update for tornado 6.4 (jupyter-server#1372)

* Support async Authorizers (jupyter-server#1373)

* Publish 2.12.0

SHA256 hashes:

jupyter_server-2.12.0-py3-none-any.whl: 3482912efa4387bb1edc23ba60531796aff3b6d6a6e93a5810f5719e2bdb48b7

jupyter_server-2.12.0.tar.gz: 9fa74ed3bb931cf33f42b3d9046e2788328ec9e6dcc59d48aa3e0910a491e3e4

* Bump to 2.13.0.dev0

* log extension import time at debug level unless it's actually slow (jupyter-server#1375)

* Add support for async Authorizers (part 2) (jupyter-server#1374)

* Publish 2.12.1

SHA256 hashes:

jupyter_server-2.12.1-py3-none-any.whl: fd030dd7be1ca572e4598203f718df6630c12bd28a599d7f1791c4d7938e1010

jupyter_server-2.12.1.tar.gz: dc77b7dcc5fc0547acba2b2844f01798008667201eea27c6319ff9257d700a6d

* Bump to 2.13.0.dev0

* Use ruff docstring-code-format (jupyter-server#1377)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Enable htmlzip and epub on readthedocs (jupyter-server#1379)

* Update pre-commit deps (jupyter-server#1380)

* Fix a typo in error message (jupyter-server#1381)

* Force legacy ws subprotocol when using gateway (jupyter-server#1311)

Co-authored-by: Emmanuel Pignot <emmanuel.pignot@netapp.com>
Co-authored-by: Zachary Sailer <zachsailer@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Zachary Sailer <zsailer@apple.com>

* Publish 2.12.2

SHA256 hashes:

jupyter_server-2.12.2-py3-none-any.whl: abcfa33f98a959f908c8733aa2d9fa0101d26941cbd49b148f4cef4d3046fc61

jupyter_server-2.12.2.tar.gz: 5eae86be15224b5375cdec0c3542ce72ff20f7a25297a2a8166a250bb455a519

* Bump to 2.13.0.dev0

* Fix test param for pytest-xdist (jupyter-server#1382)

* Simplify the jupytext downstream test (jupyter-server#1383)

* Import User unconditionally (jupyter-server#1384)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Publish 2.12.3

SHA256 hashes:

jupyter_server-2.12.3-py3-none-any.whl: 6f85310ea5e6068568a521f079fba99d8d17e4884dd1d602ab0f43b3115204a8

jupyter_server-2.12.3.tar.gz: a1d2d51e497b1a6256c48b6940b0dd49b2553981baf1690077c37792f1fa23a1

* Bump to 2.13.0.dev0

* Fix log arguments for gateway client error (jupyter-server#1385)

* Publish 2.12.4

SHA256 hashes:

jupyter_server-2.12.4-py3-none-any.whl: a125ae18a60de568f78f55c84dd58759901a18ef279abf0418ac220653ca1320

jupyter_server-2.12.4.tar.gz: 41f4a1e6b912cc24a7c6c694851b37d3d8412b180f43d72315fe422cb2b85cc2

* merge 2.12.4 from upstream

* merge 2.12.4 from upstream

---------

Co-authored-by: Lironavr1 <107797717+Lironavr1@users.noreply.github.com>
Co-authored-by: Sam Bloomquist <bloomquist.sam@gmail.com>
Co-authored-by: Steven Silvester <steven.silvester@ieee.org>
Co-authored-by: blink1073 <blink1073@users.noreply.github.com>
Co-authored-by: IITII <ccmejx@gmail.com>
Co-authored-by: Zhongsheng Ji <9573586@qq.com>
Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>
Co-authored-by: Min RK <benjaminrk@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Zachary Sailer <zsailer@apple.com>
Co-authored-by: Zsailer <Zsailer@users.noreply.github.com>
Co-authored-by: Nicholas Bollweg <nick.bollweg@gmail.com>
Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
Co-authored-by: Manu <21658174+epignot@users.noreply.github.com>
Co-authored-by: Emmanuel Pignot <emmanuel.pignot@netapp.com>
Co-authored-by: Zachary Sailer <zachsailer@gmail.com>
Co-authored-by: Gonzalo Tornaría <tornaria@gmail.com>
Co-authored-by: Marc Wouts <marc.wouts@gmail.com>
Co-authored-by: Yuvi Panda <yuvipanda@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Latest release is breaking custom ContentManager
4 participants