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

Ensure temp_url_key setting is the correct type for use as HMAC key #72

Merged
merged 1 commit into from
Jan 11, 2017

Conversation

pomegranited
Copy link
Contributor

@pomegranited pomegranited commented Jan 9, 2017

This change assists with the generation of temporary URLs for private SWIFT content, by ensuring that the SWIFT_TEMP_URL_KEY is converted to a (non-unicode) string.

If SWIFT_TEMP_URL_KEY is a unicode string, which happens if django app settings are parsed from JSON, then an error like this one is thrown during temporary URL creation: HMAC TypeError: character mapping must return integer, None or unicode

Manual Testing instructions:

Setup:

  1. Create a file called 'env.json', which contains your SWIFT credentials and container name, e.g.,

    {
      "SWIFT_AUTH_URL": "https://auth.cloud.ovh.net/v2.0",
      "SWIFT_USERNAME": "xxx",
      "SWIFT_PASSWORD": "xxx",
      "SWIFT_TENANT_NAME": "xxx",
      "SWIFT_CONTAINER_NAME": "xxx",
      "SWIFT_USE_TEMP_URLS": true,
      "SWIFT_TEMP_URL_KEY": "xxx",
      "SWIFT_FILE_TO_SHARE": "file.txt"
    }
    
  2. In the same location as env.json, create this web.py script to run a minimal Django app:

  #### Setup

  from django.conf import settings
  import json

  with open("env.json") as env_file:
      ENV_TOKENS = json.load(env_file)

  if not settings.configured:
      settings.configure(
          DEBUG = True,
          ROOT_URLCONF = 'web',
          SWIFT_AUTH_URL = ENV_TOKENS['SWIFT_AUTH_URL'],
          SWIFT_USERNAME = ENV_TOKENS['SWIFT_USERNAME'],
          SWIFT_PASSWORD = ENV_TOKENS['SWIFT_PASSWORD'],
          SWIFT_TENANT_NAME = ENV_TOKENS['SWIFT_TENANT_NAME'],
          SWIFT_CONTAINER_NAME = ENV_TOKENS['SWIFT_CONTAINER_NAME'],
          SWIFT_USE_TEMP_URLS = ENV_TOKENS['SWIFT_USE_TEMP_URLS'],
          SWIFT_TEMP_URL_KEY = ENV_TOKENS['SWIFT_TEMP_URL_KEY'],
          SWIFT_FILE_TO_SHARE = ENV_TOKENS['SWIFT_FILE_TO_SHARE'],
      )

  from django.conf.urls import patterns
  urlpatterns = patterns('',
      (r'^$', 'web.index'),
  )

  #### Handlers

  from django.http import HttpResponse
  from swift.storage import SwiftStorage

  def index(request):
      ss = SwiftStorage()
      url = ss.url(settings.SWIFT_FILE_TO_SHARE)
      return HttpResponse(url)

  #### Running

  if __name__ == '__main__':
      from django.core.management import execute_from_command_line
      execute_from_command_line()

To verify the error:

  1. Run pip install django django-storage-swift
  2. Run python web.py runserver
  3. Visit http://127.0.0.1:8000. Note the error: TypeError: character mapping must return integer, None or unicode

To verify this change:

  1. Build and install the branch from this PR: python setup.py sdist && pip install dist/dist/django-storage-swift-1.2.15.tar.gz
  2. Run python web.py runserver
  3. Visit http://127.0.0.1:8000/. Note that a valid temporary URL is displayed.

@haikuginger
Copy link
Contributor

@pomegranited, just saw this cross my inbox and wanted to double-check: does this work in Python3? If this needs a bytes-like object, you'd want to do .encode(), not str().

@einarf
Copy link
Contributor

einarf commented Jan 10, 2017

@haikuginger @pomegranited Write a quick test in tests/tests.py and this will be fairly easy to detect when travis runs :)

The temp-url feature is one of the very few things that are not in existing tests, but coverage is at least 90%+.

I can help with that if needed. Just poke me.

@pomegranited pomegranited force-pushed the jill/temp_url_settings branch 3 times, most recently from 544b6cf to 0e7b782 Compare January 10, 2017 07:18
@pomegranited
Copy link
Contributor Author

Thanks for the pointer, @haikuginger !

@pomegranited
Copy link
Contributor Author

@einarf Apologies for the spam and rebasing.. somehow I missed your test/ dir entirely on my first commit! Tests added, and I ended up changing it a bit to handle more use cases.

How does it look now?

Re 6b21aff - Do you know of a better way to test that case?

@einarf
Copy link
Contributor

einarf commented Jan 10, 2017

@pomegranited Tests looks fine at first glance. TBH just having someone caring to write tests is a miracle on its own 👏

For the last test: You could compare the input value type with six.binary_type. That will be str in py2 and bytes in py3. (isinstance(..))

EDIT: You could also just specify ascii when doing the encode: encode('ascii'). That will raise a UnicodeDecodeError in both python 2 and 3.

The reason why the test do not fail in py3 is that encode converts the string into a bytes structure using an utf-8 codec by default while py2 uses an ascii codec by default to convert a unicode string into str.

try:
backend.temp_url_key = backend.temp_url_key.encode('latin-1')
backend.temp_url_key = backend.temp_url_key.encode()
Copy link
Contributor

Choose a reason for hiding this comment

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

encode('ascii') so python 3 don't use utf-8. This will of course assume that we always configure the key as a string. I think that's acceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@einarf Cool! I've done that with d2a1405, which let me re-add that test.

@pomegranited
Copy link
Contributor Author

@einarf Thank you for your help, and for building a great django-storage package!

Let me know if you're happy with this PR, and I'll squash my commits.

@einarf
Copy link
Contributor

einarf commented Jan 11, 2017

@pomegranited Blame @Blacktorn for this package, not me. He will probable stop by and merge this very soon. He normally use the auto-squash feature here in github so you don't have to think about that.

This looks ready for merge as far as I can see. 👍

...so it can be used as the HMAC key.

Also adds tests for temp URLs.
@pomegranited pomegranited force-pushed the jill/temp_url_settings branch from d2a1405 to 09bcd69 Compare January 11, 2017 08:46
@pomegranited
Copy link
Contributor Author

@einarf Birds gotta sing, I've gotta squash. Rebased from d2a1405 😄

@dennisv
Copy link
Owner

dennisv commented Jan 11, 2017

Thanks for the changes and also for writing the temp url tests @pomegranited !

@dennisv dennisv merged commit 043b578 into dennisv:master Jan 11, 2017
@dennisv
Copy link
Owner

dennisv commented Jan 11, 2017

I've also pushed the v1.2.15 release to PyPI

@pomegranited
Copy link
Contributor Author

Thank you, @Blacktorn and @einarf !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants