Skip to content

Conversation

salian
Copy link
Contributor

@salian salian commented Jan 10, 2016

Addresses issue #268

Overrides perform_update (similar to perform_create) to update Device.user when updating existing devices.

@salian salian changed the title Update Device.user when updating existing devices. Update Device.user when updating existing devices via DRF. Jan 10, 2016
@jamaalscarlett
Copy link
Member

@salian This looks fine, although there is an inherent flaw in the un-authenticated viewset. Currently, anyone can edit any object. All that is needed is the registration_id, once that is know any of the serializer fields can modified.

@salian
Copy link
Contributor Author

salian commented Jan 10, 2016

@jamaalscarlett I agree, especially in combination with an un-authenticated endpoint for exposing a list of all devices via GET. It takes away whatever little security via obscurity there is.
I suppose I shall want to exclude the RetrieveModelMixin from the un-authenticated view GCMDeviceViewSetso it doesn't give out a list of all my registration IDs.

@salian salian closed this Jan 11, 2016
@salian salian deleted the patch-1 branch January 11, 2016 14:11
@salian salian restored the patch-1 branch January 11, 2016 14:11
@salian salian reopened this Jan 11, 2016
@jamaalscarlett
Copy link
Member

@salian I would be more comfortable if there was a check for an existing user. To prevent the current user for being overwritten, I would only save the user if the current user is None.

@salian
Copy link
Contributor Author

salian commented Jan 14, 2016

I think the desired behaviour is to overwrite the user in case of a duplicate registration_id.
As I understand, there is explicitly a unique validator on the registration_id field at line 71 in rest_framework.py, so having two records with the same registration_id is not an option.

Moreover GCM will issue the same registration id again unless a user uninstalls and reinstalls the app, so there is no way for the calling app to come up with a different registration_id; in which case arguably it is better to keep the fresh record than the stale one?

@jamaalscarlett
Copy link
Member

@salian I forgot about the validator. This is why a wrote my own DRF integration. Could you please squash these commits. After that, this is good to merge.

@jamaalscarlett
Copy link
Member

@salian can you clean up the commit so we can get this merged in?

@jamaalscarlett
Copy link
Member

@jleclanche any thoughts on this one?

@salian
Copy link
Contributor Author

salian commented Feb 4, 2016

@jamaalscarlett I actually can't figure how to clean it via the web interface, and I am not familiar with the CLI. Should I delete this and make a fresh branch?

@jamaalscarlett
Copy link
Member

@salian From the command line run git rebase -i HEAD~3 and follow the instructions.

@jamaalscarlett
Copy link
Member

@salian any luck? I can fix it if you still can't figure it out.

@jleclanche
Copy link
Member

@jamaalscarlett gtm; you can rebase & land. sorry this went under the radar.

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.

3 participants