-
Notifications
You must be signed in to change notification settings - Fork 61
Fix #158: Update JSON formats to latest OBI spec #195
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
|
||
from datetime import datetime, timedelta, tzinfo | ||
from time import time, gmtime, strftime | ||
import calendar | ||
|
||
import os.path | ||
from os.path import dirname | ||
|
@@ -29,10 +30,7 @@ | |
|
||
from django.core.serializers.json import DjangoJSONEncoder | ||
|
||
try: | ||
import django.utils.simplejson as json | ||
except ImportError: # Django 1.5 no longer bundles simplejson | ||
import json | ||
import json | ||
|
||
# HACK: Django 1.2 is missing receiver and user_logged_in | ||
try: | ||
|
@@ -87,10 +85,10 @@ | |
IMG_MAX_SIZE = getattr(settings, "BADGER_IMG_MAX_SIZE", (256, 256)) | ||
|
||
SITE_ISSUER = getattr(settings, 'BADGER_SITE_ISSUER', { | ||
"origin": "http://mozilla.org", | ||
"name": "Badger", | ||
"org": "Mozilla", | ||
"contact": "lorchard@mozilla.com" | ||
"name": "Example", | ||
"url": "http://example.com", | ||
"description": "This is an example organization", | ||
"email": "me@example.com" | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having these settings at module-level so that they populate at module-import time is ... not great. That's probably something we should fix, too. |
||
|
||
# Set up a file system for badge uploads that can be kept separate from the | ||
|
@@ -446,7 +444,9 @@ class Meta: | |
def __unicode__(self): | ||
return self.title | ||
|
||
def get_absolute_url(self): | ||
def get_absolute_url(self, format='html'): | ||
if format == 'json': | ||
return reverse('badger.detail_json', args=(self.slug,)) | ||
return reverse('badger.views.detail', args=(self.slug,)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like we could do these patterns better and have them switch back and forth based on an argument. But ... that's under-the-hood, so we can tinker with that later. |
||
|
||
def get_upload_meta(self): | ||
|
@@ -653,32 +653,21 @@ def as_obi_serialization(self, request=None): | |
else: | ||
base_url = 'http://%s' % (Site.objects.get_current().domain,) | ||
|
||
# see: https://github.com/brianlovesdata/openbadges/wiki/Assertions | ||
if not self.creator: | ||
issuer = SITE_ISSUER | ||
else: | ||
issuer = { | ||
# TODO: Get from user profile instead? | ||
"origin": urljoin(base_url, self.creator.get_absolute_url()), | ||
"name": self.creator.username, | ||
"contact": self.creator.email | ||
} | ||
|
||
data = { | ||
# The version of the spec/hub this manifest is compatible with. Use | ||
# "0.5.0" for the beta. | ||
"version": OBI_VERSION, | ||
# TODO: truncate more intelligently | ||
"name": self.title[:128], | ||
# TODO: truncate more intelligently | ||
"description": self.description[:128] or self.title[:128], | ||
"criteria": urljoin(base_url, self.get_absolute_url()), | ||
"issuer": issuer | ||
"issuer": urljoin(base_url, reverse('badger.site_issuer')) | ||
} | ||
|
||
image_url = self.image and self.image.url or DEFAULT_BADGE_IMAGE_URL | ||
data['image'] = urljoin(base_url, image_url) | ||
|
||
# TODO: tags | ||
# TODO: alignment | ||
|
||
return data | ||
|
||
|
||
|
@@ -720,7 +709,9 @@ def __unicode__(self): | |
return u'Award of %s to %s%s' % (self.badge, self.user, by) | ||
|
||
@models.permalink | ||
def get_absolute_url(self): | ||
def get_absolute_url(self, format='html'): | ||
if format == 'json': | ||
return ('badger.award_detail_json', (self.badge.slug, self.pk)) | ||
return ('badger.views.award_detail', (self.badge.slug, self.pk)) | ||
|
||
def get_upload_meta(self): | ||
|
@@ -788,40 +779,38 @@ def delete(self): | |
super(Award, self).delete() | ||
|
||
def as_obi_assertion(self, request=None): | ||
badge_data = self.badge.as_obi_serialization(request) | ||
|
||
"""Build a representation of this award as an OBI assertion""" | ||
if request: | ||
base_url = request.build_absolute_uri('/')[:-1] | ||
else: | ||
base_url = 'http://%s' % (Site.objects.get_current().domain,) | ||
|
||
# If this award has a creator (ie. not system-issued), tweak the issuer | ||
# data to reflect award creator. | ||
# TODO: Is this actually a good idea? Or should issuer be site-wide | ||
if self.creator: | ||
badge_data['issuer'] = { | ||
# TODO: Get from user profile instead? | ||
"origin": base_url, | ||
"name": self.creator.username, | ||
"contact": self.creator.email | ||
} | ||
|
||
# see: https://github.com/brianlovesdata/openbadges/wiki/Assertions | ||
# TODO: This salt is stable, and the badge.pk is generally not | ||
# disclosed anywhere, but is it obscured enough? | ||
hash_salt = (hashlib.md5('%s-%s' % (self.badge.pk, self.pk)) | ||
hash_salt = (hashlib.md5('%s-%s-%s' % (self.badge.pk, | ||
self.pk, | ||
settings.SECRET_KEY)) | ||
.hexdigest()) | ||
recipient_text = '%s%s' % (self.user.email, hash_salt) | ||
recipient_hash = ('sha256$%s' % hashlib.sha256(recipient_text) | ||
.hexdigest()) | ||
assertion = { | ||
"recipient": recipient_hash, | ||
"salt": hash_salt, | ||
"uid": '%s' % self.id, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like a bad idea. If we ever need to move badges from one system to another, we'd have to keep their ids intact and that is problematic. This is probably something we should fix at some point. |
||
"recipient": { | ||
"type": "email", | ||
"hashed": True, | ||
"salt": hash_salt, | ||
"identity": recipient_hash | ||
}, | ||
"badge": urljoin(base_url, | ||
self.badge.get_absolute_url(format='json')), | ||
"verify": { | ||
"type": "hosted", | ||
"url": urljoin(base_url, | ||
self.get_absolute_url(format='json')) | ||
}, | ||
"evidence": urljoin(base_url, self.get_absolute_url()), | ||
"issuedOn": calendar.timegm(self.created.utctimetuple()), | ||
# TODO: implement award expiration | ||
# "expires": self.expires.date().isoformat(), | ||
"issued_on": self.created.date().isoformat(), | ||
"badge": badge_data | ||
# "expires": calendar.timegm(self.expires.utctimetuple()), | ||
} | ||
return assertion | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,13 +8,17 @@ | |
except ImportError: | ||
import Image | ||
|
||
from urlparse import urljoin | ||
import hashlib | ||
|
||
from django.conf import settings | ||
|
||
from django.core.management import call_command | ||
from django.db.models import loading | ||
from django.contrib.sites.models import Site | ||
from django.core.files.base import ContentFile | ||
from django.http import HttpRequest | ||
from django.utils import simplejson as json | ||
import json | ||
from django.test.client import Client | ||
|
||
from django.core import mail | ||
|
@@ -44,7 +48,8 @@ | |
NominationApproveNotAllowedException, | ||
NominationAcceptNotAllowedException, | ||
NominationRejectNotAllowedException, | ||
SITE_ISSUER, slugify) | ||
SITE_ISSUER, DEFAULT_BADGE_IMAGE_URL, | ||
slugify) | ||
|
||
from badger_example.models import GuestbookEntry | ||
|
||
|
@@ -116,6 +121,59 @@ def test_award_unique_duplication(self): | |
|
||
class BadgerOBITest(BadgerTestCase): | ||
|
||
def test_badge_class_data(self): | ||
|
||
# Make a badge with a creator | ||
user_creator = self._get_user(username="creator") | ||
badge = self._get_badge(title="Badge with Creator", | ||
creator=user_creator) | ||
|
||
base_url = 'http://%s' % (Site.objects.get_current().domain,) | ||
|
||
obi = badge.as_obi_serialization() | ||
eq_(obi['name'], badge.title[:128]) | ||
eq_(obi['description'], badge.description[:128] or self.title[:128]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given we're setting this test up, we should be able to know whether it'll end up being the description or the title. Plus what is "self.title" here? Should that be "badge.title"? |
||
eq_(obi['image'], urljoin(base_url, DEFAULT_BADGE_IMAGE_URL)) | ||
eq_(obi['criteria'], urljoin(base_url, badge.get_absolute_url())) | ||
eq_(obi['issuer'], | ||
urljoin(base_url, reverse('badger.site_issuer'))) | ||
# TODO: tags | ||
# TODO: alignment | ||
|
||
def test_badge_assertion_data(self): | ||
user_creator = self._get_user(username="creator") | ||
user_awardee = self._get_user(username="awardee_1") | ||
|
||
badge = self._get_badge(title="Badge with Creator", | ||
creator=user_creator) | ||
award = badge.award_to(awardee=user_awardee) | ||
|
||
obi = award.as_obi_assertion() | ||
|
||
base_url = 'http://%s' % (Site.objects.get_current().domain,) | ||
|
||
eq_(obi['uid'], '%s' % award.id) | ||
|
||
hash_salt = obi['recipient']['salt'] | ||
recipient_text = '%s%s' % (award.user.email, hash_salt) | ||
recipient_hash = ('sha256$%s' % hashlib.sha256(recipient_text) | ||
.hexdigest()) | ||
eq_(obi['recipient']['type'], 'email') | ||
ok_(obi['recipient']['hashed']) | ||
eq_(obi['recipient']['identity'], recipient_hash) | ||
|
||
eq_(obi['badge'], | ||
urljoin(base_url, badge.get_absolute_url(format='json'))) | ||
|
||
eq_(obi['verify']['type'], 'hosted') | ||
eq_(obi['verify']['url'], | ||
urljoin(base_url, award.get_absolute_url(format='json'))) | ||
|
||
eq_(type(obi['issuedOn']), int) | ||
|
||
eq_(obi['evidence'], | ||
urljoin(base_url, award.get_absolute_url())) | ||
|
||
def test_baked_award_image(self): | ||
"""Award gets image baked with OBI assertion""" | ||
# Get the source for a sample badge image | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ | |
|
||
urlpatterns = patterns('badger.views', | ||
url(r'^$', 'badges_list', name='badger.badges_list'), | ||
url(r'^issuer.json$', 'site_issuer', name='badger.site_issuer'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this have to live at the root of a site? e.g. https://support.mozilla.org/issuer.json There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After reading the rest of this PR, I think the answer here is "no". The issuer url in question is in the assertion, so whatever is there is fine. It does bring up the issue where if you change the issuer url, I think all your prior assertions are now screwed. If that's true, we might want to note that in the documentation. Something like, "Don't change the url for your assertion. Srsly--it'd be bad." I wonder how that works for sites that switch from http to https-only. |
||
url(r'^staff_tools$', 'staff_tools', | ||
name='badger.staff_tools'), | ||
url(r'^tag/(?P<tag_name>.+)/?$', 'badges_list', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oo--good changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having said that, part of me thinks we shouldn't provide a default here. Instead we should throw a YOIMPROPERLYCONFIGUREDERRORRZ kind of thing.