Skip to content

chore: Upgrade to Django 4.2 - #5665

Merged
jennifer-richards merged 28 commits into
ietf-tools:feat/django4from
jennifer-richards:django42
May 23, 2023
Merged

chore: Upgrade to Django 4.2#5665
jennifer-richards merged 28 commits into
ietf-tools:feat/django4from
jennifer-richards:django42

Conversation

@jennifer-richards

Copy link
Copy Markdown
Member

This brings us to the current version of Django.

Tests all pass and all but one deprecation warning have been resolved or suppressed after creating a ticket to track eventual resolution. The one exception, caused by a GET to the OIDC end-session view, will need to be resolved in django-oidc-provider and has simply been suppressed. When we get to Django 5, we should re-enable the warning and run tests to be sure we are clear of the problem.

A few significant changes:

The session serializer has been changed to JSONSerializer. There was one problem with this - the nomcom private key was being stored in the session as bytes, which cannot be serialized directly as JSON. I fixed this by decoding as utf8 before stuffing into the session and encoding as utf8 when retrieving.

Django 5 will change the default rendering of formsets to use a div-based structure. I've opted in to adopting this change early by setting FORM_RENDERER in settings.py. I checked a few places to see that this did not affect our views, but we should probably check more thoroughly. Hopefully the test crawl will catch problems if there are any.

Logging out via GET has been deprecated. The "Sign out" button in the dropdown menu has been changed to a form with a button to POST to the logout view. This worked without needing to modify CSS, but a few tests that counted the number of forms or number of "submit" buttons needed to be tweaked to ignore the sign out form.

pkg_resources warning is caused by a few packages
(django-simple-history. django-widget-tweaks, etc). The datetime_safe
warning is in tastypie, as indicated. The others are deprecated
settings we already have tickets for.
PickleSerializer is deprecated
(the "Sign out" link is now a submit button)
bytes are incompatible with JSONSerializer
@richsalz

Copy link
Copy Markdown
Collaborator

There was one problem with this - the nomcom private key was being stored in the session as bytes, which cannot be serialized directly as JSON. I fixed this by decoding as utf8 before stuffing into the session and encoding as utf8 when retrieving.

I do not know enough about how the system works, but let me raise the issue. The NomCom private key isn't characters, it's an array of bytes and trying to make it into and out-of UTF8 is probably not safe. If you need to turn key material into json, the best thing is to do to/from base64.

Hope this helps.

@jennifer-richards

jennifer-richards commented May 19, 2023

Copy link
Copy Markdown
Member Author

There was one problem with this - the nomcom private key was being stored in the session as bytes, which cannot be serialized directly as JSON. I fixed this by decoding as utf8 before stuffing into the session and encoding as utf8 when retrieving.

I do not know enough about how the system works, but let me raise the issue. The NomCom private key isn't characters, it's an array of bytes and trying to make it into and out-of UTF8 is probably not safe. If you need to turn key material into json, the best thing is to do to/from base64.

Hope this helps.

Thanks for the comment - I think it's in encrypted, ascii-armored state (i.e., already base64 coded) when it's put in the session store. In the tests, the value in the session is faked with this:

key = b'''-----BEGIN PRIVATE KEY-----

In actual operation, it's stored here

command = "%s bf -e -in /dev/stdin -k \"%s\" -a"

and retrieved here

command = "%s bf -d -in /dev/stdin -k \"%s\" -a"

(the first %s in both command lines is openssl, so those are blowfish encryption and decryption command lines)

The output of that command that was being handled as bytes because that's how we get it from pipe. I think it should always be ascii and the change is more about bookkeeping in Python's string handling.

@codecov

codecov Bot commented May 19, 2023

Copy link
Copy Markdown

Codecov Report

Merging #5665 (ceb41e6) into feat/django4 (55fb502) will increase coverage by 0.00%.
The diff coverage is 66.66%.

@@              Coverage Diff              @@
##           feat/django4    #5665   +/-   ##
=============================================
  Coverage         88.73%   88.73%           
=============================================
  Files               287      287           
  Lines             39869    39876    +7     
=============================================
+ Hits              35378    35385    +7     
  Misses             4491     4491           
Impacted Files Coverage Δ
ietf/stats/models.py 83.33% <0.00%> (-2.04%) ⬇️
ietf/utils/jstest.py 47.14% <0.00%> (ø)
ietf/nomcom/templatetags/nomcom_tags.py 67.18% <50.00%> (-1.67%) ⬇️
ietf/nomcom/utils.py 91.30% <50.00%> (ø)
ietf/doc/views_search.py 89.25% <100.00%> (ø)
ietf/nomcom/models.py 91.73% <100.00%> (ø)
ietf/nomcom/views.py 96.77% <100.00%> (+0.01%) ⬆️
ietf/review/utils.py 89.57% <100.00%> (ø)

... and 3 files with indirect coverage changes

Comment thread ietf/nomcom/utils.py
command_line_safe_secret(settings.NOMCOM_APP_SECRET)
),
private_key
private_key.encode("utf8")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this is going to work. The nomcom private key is part of a keypair that is generated externally, and the byte sequence is not guaranteed to be valid utf8. Maybe uuencoding first would work around the issue?

Please also check that the instructions (and explanations) at https://github.com/ietf-tools/datatracker/blob/main/ietf/templates/nomcom/chair_help.html#L9-L76 are still accurate.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry - I hadn't seen the exchange with Rich

@jennifer-richards jennifer-richards May 23, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This should not change anything with respect to the instructions or explanations. The change here is only in handling the data that comes back from gpg openssl. As long as the reverse operation is done when feeding it back to gpg openssl to decrypt, it should be transparent.

I will see about switching it for a more robust handling of arbitrary bytes, otherwise someone somewhere will change a locale and we'll get a weird bug.

@jennifer-richards
jennifer-richards merged commit 22e80ce into ietf-tools:feat/django4 May 23, 2023
@jennifer-richards
jennifer-richards deleted the django42 branch May 23, 2023 15:09
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators May 27, 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.

3 participants