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

Remove runtime javascript minification #6425

Merged
merged 1 commit into from
Jun 15, 2022
Merged

Remove runtime javascript minification #6425

merged 1 commit into from
Jun 15, 2022

Conversation

zenmonkeykstop
Copy link
Contributor

@zenmonkeykstop zenmonkeykstop commented May 3, 2022

Status

Ready for review

Description of Changes

Fixes #6415

  • serves static/js resources instead of gen/js resources
  • removes flask-assets, jsmin, and webassets dependencies and related code
  • updates apparmor profile to remove rw access to /gen/
  • updates builder functionality/tests to ensure gen/ not present
  • updates postinst to remove gen/, .webassets-cache/ if present

Testing

  • checkout this branch
  • run git clean -dfx to ensure old generated files aren't left hanging around
  • run make dev-tor
    • confirm that applications start successfully
    • confirm that source.js on the SI home page is served from /static/js/, not /static/gen
    • visit the SI via a tor2web proxy and verify that you are immediately redirected to the /tor2web-warning page on visiting the SI home page
    • log into the JI and confirm that journalst.js on the JI All Source page is served from /static/js/, not /static/gen
    • confirm that the source filter still works
  • run make build-debs, confirm that the package build completes without error
  • run make staging, and confirm that staging servers are provisioned without error and that source.js and journalist.js are served from /static/js on the SI and JI respectively

Upgrade testing (optional)

  • run through the upgrade scenario with this branch and confirm that the upgrade completes without error and that source.js and journalist.js are served from /static/js on the SI and JI respectively

Deployment

This PR modifies the securedrop-app-code postint to remove the /static/gen and /static/.webassets-cache directories in the webroot, so upgrade testing should be performed prior to release.

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make test) pass in the development container

If you made changes to securedrop-admin:

  • Linting and tests (make -C admin test) pass in the admin development container

If you added or removed a file deployed with the application:

  • I have updated AppArmor rules to include the change

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

Choose one of the following:

  • I have opened a PR in the docs repo for these changes, or will do so later
  • I would appreciate help with the documentation
  • These changes do not require documentation

@codecov-commenter
Copy link

codecov-commenter commented May 4, 2022

Codecov Report

Merging #6425 (8a4f055) into develop (585a03a) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #6425      +/-   ##
===========================================
- Coverage    83.83%   83.81%   -0.02%     
===========================================
  Files           62       62              
  Lines         4331     4326       -5     
  Branches       525      525              
===========================================
- Hits          3631     3626       -5     
  Misses         575      575              
  Partials       125      125              
Impacted Files Coverage Δ
securedrop/journalist_app/__init__.py 88.88% <ø> (-0.19%) ⬇️
securedrop/source_app/__init__.py 90.21% <ø> (-0.31%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@legoktm
Copy link
Member

legoktm commented May 16, 2022

(rebased on top of develop so the /tor2web-warning endpoint isn't throwing 500 errors, was fixed in cfae1e6)

Copy link
Member

@legoktm legoktm left a comment

Choose a reason for hiding this comment

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

debs are still building, but I think we can remove the gen and .webassets-cache folders from the "Create static asset directories" step.

Copy link
Member

@legoktm legoktm left a comment

Choose a reason for hiding this comment

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

This is the build failure I got:

    =================================== FAILURES ===================================
    _ test_deb_package_contains_no_generated_assets[docker://focal-sd-dpkg-verification] _
    [gw1] linux -- Python 3.7.3 /home/user/securedrop/.venv/bin/python3
    
    securedrop_app_code_contents = 'drwxr-xr-x root/root         0 2022-05-06 14:05 ./\ndrwxr-xr-x root/root         0 2022-05-06 14:05 ./etc/\ndrwxr-xr-...n3 -> /usr/bin/python3\nlrwxrwxrwx root/root         0 2022-05-06 14:05 ./opt/venvs/securedrop-app-code/lib64 -> lib\n'
    
        def test_deb_package_contains_no_generated_assets(securedrop_app_code_contents: str):
            """
            Ensures the `securedrop-app-code` package does not ship minified
            static assets previously built at runtime via Flask-Assets under /gen, and
            which may be present in the source directory used to build from.
            """
            # static/gen/ directory not should exist
    >       assert not re.search(
                r"^.*\./var/www/securedrop" "/static/gen/$", securedrop_app_code_contents, re.M
            )
    E       AssertionError: assert not <re.Match object; span=(523650, 523730), match='drwxr-xr-x root/root         0 2022-05-06 14:05 .>
    E        +  where <re.Match object; span=(523650, 523730), match='drwxr-xr-x root/root         0 2022-05-06 14:05 .> = <function search at 0x7080ed8d21e0>('^.*\\./var/www/securedrop/static/gen/$', 'drwxr-xr-x root/root         0 2022-05-06 14:05 ./\ndrwxr-xr-x root/root         0 2022-05-06 14:05 ./etc/\ndrwxr-xr-...n3 -> /usr/bin/python3\nlrwxrwxrwx root/root         0 2022-05-06 14:05 ./opt/venvs/securedrop-app-code/lib64 -> lib\n', <RegexFlag.MULTILINE: 8>)
    E        +    where <function search at 0x7080ed8d21e0> = re.search
    E        +    and   <RegexFlag.MULTILINE: 8> = re.M
    
    tests/test_securedrop_deb_package.py:273: AssertionError
    =========================== short test summary info ============================
    FAILED tests/test_securedrop_deb_package.py::test_deb_package_contains_no_generated_assets[docker:/focal-sd-dpkg-verification]
    ======= 1 failed, 143 passed, 1 skipped, 1 xfailed in 143.10s (0:02:23) ========

- serves static/js resources instead of gen/js resources
- removes flask-assets, jsmin, and webassets dependencies and related code
- updated apparmor profile to remove rw access to /gen/
- updates builder functionality/tests to ensure gen/ not present
- updates postinst to remove gen/, .webassets-cache/ if present
@zenmonkeykstop
Copy link
Contributor Author

D'oh! apologies on missing the CR @legoktm, PR is updated now.

Copy link
Member

@legoktm legoktm left a comment

Choose a reason for hiding this comment

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

Fantastic, LGTM!

@legoktm legoktm merged commit e8afb2f into develop Jun 15, 2022
@legoktm legoktm deleted the 6415-remove-jsmin branch June 15, 2022 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove JS minification from the web applications.
3 participants