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

Add management command to increase an ODKTokens lifetime #1847

Merged
merged 2 commits into from
Jul 30, 2020

Conversation

DavisRayM
Copy link
Contributor

@DavisRayM DavisRayM commented Jul 16, 2020

Changes / Features implemented

  • Added expires date-time field for ODKToken class
  • Added a management command increase_odk_token_lifetime

Steps taken to verify this change does what is intended

  • Added tests

Side effects of implementing this change

  • All ODKTokens created before this change will not have the expires date set. Which might cause issues. All ODKTokens should be invalidated/regenerated.

Closes #1747

@DavisRayM DavisRayM force-pushed the elongate-odk-token-command branch 6 times, most recently from d8b677d to 290d29a Compare July 17, 2020 11:48
@DavisRayM DavisRayM changed the title [WIP] Add management command to elongate the odk token [WIP] Add management command to increase an ODKTokens lifetime Jul 17, 2020
@DavisRayM DavisRayM force-pushed the elongate-odk-token-command branch 2 times, most recently from 99e73a7 to 6cf5edc Compare July 17, 2020 11:58
@DavisRayM DavisRayM changed the title [WIP] Add management command to increase an ODKTokens lifetime Add management command to increase an ODKTokens lifetime Jul 17, 2020
@DavisRayM DavisRayM marked this pull request as ready for review July 17, 2020 12:37
@DavisRayM DavisRayM force-pushed the elongate-odk-token-command branch 3 times, most recently from 1f826dd to dcfed84 Compare July 30, 2020 06:03
Copy link
Contributor

@ivermac ivermac left a comment

Choose a reason for hiding this comment

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

Generally looks good. I've added a few queries

Comment on lines 99 to 101
instance.expires = instance.created + timedelta(
days=ODK_TOKEN_LIFETIME)
instance.expires = instance.expires.astimezone(
instance.created.tzinfo)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we overriding instance.expires in line 101? Also, will instance.expires in line 99 still have the value assigned to it even though it hasn't been saved and it's being used in line 101? Let me know if my questions make sense

Copy link
Contributor Author

@DavisRayM DavisRayM Jul 30, 2020

Choose a reason for hiding this comment

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

Overriding the value to make sure that the date-time object saved in the field is timezone aware(Usually Django saves it's datetime objects with the UTC timezone when Timezones are enabled. Didn't want to deviate from that)

The instance will still have the expires value assigned to the object in line 99 and the value will be accessible in line 101, I believe. I'll update this though and set the expires field in one line, just in case.

Comment on lines 134 to 147
if username:
if isinstance(form_id, XForm):
root = form_id.survey.name
else:
try:
form = XForm.objects.filter(
id_string__iexact=form_id,
user__username__iexact=username,
deleted_at__isnull=True).first()
root = form.survey.name
except XForm.DoesNotExist:
root = 'data'
else:
root = 'data'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have we changed the value assigned to root from form_id (which seems to be an xform object) to a string value (data)? Also, i'm not very conversant with this part of the code. I'd like @ukanga's eye on this change as well

Copy link
Contributor Author

@DavisRayM DavisRayM Jul 30, 2020

Choose a reason for hiding this comment

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

My bad, these are changes meant for another pull request. I didn't notice the commits were rebased into this one. I'll update the PR

The 'expires' field will enable one to increment / decrement the expiry
date of an ODKToken easily. Included the '_post_save_set_expiry_date'
function to set the 'expires' field value post_save in order to have
access to the 'created' field value

- Update 'test_fails_authentication_past_odk_token_expiry' test
@ivermac ivermac self-requested a review July 30, 2020 07:06
@DavisRayM DavisRayM merged commit 2ad7a84 into master Jul 30, 2020
@DavisRayM DavisRayM deleted the elongate-odk-token-command branch July 30, 2020 14:03
@DavisRayM DavisRayM mentioned this pull request Aug 10, 2020
1 task
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.

Add a management command to elongate the expiry date of an ODKToken
2 participants