Skip to content

Conversation

@hbradio
Copy link
Collaborator

@hbradio hbradio commented Aug 8, 2019

This makes the websocket security check allow shared users. I realized I forgotten about this in the last shared-users PR.

@hbradio hbradio requested a review from cabarnes August 8, 2019 15:16
@hbradio hbradio self-assigned this Aug 8, 2019
return

if rover.owner.id != user.id:
shared_user_ids = [user.id for user in rover.shared_users.all()]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't practice much with the Django ORM. Is there a better way to do this?

Copy link
Member

Choose a reason for hiding this comment

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

You could do something like:

is_shared_user = rover.shared_users.filter(id=user.id).exists()

@coveralls
Copy link

coveralls commented Aug 8, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 560a344 on allow-shared-ws-connect into e104cd6 on alpha.

return

if rover.owner.id != user.id:
shared_user_ids = [user.id for user in rover.shared_users.all()]
Copy link
Member

Choose a reason for hiding this comment

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

You could do something like:

is_shared_user = rover.shared_users.filter(id=user.id).exists()

local_ip='8.8.8.8',
oauth_application=oauth_app,
)
rover.shared_users.set([shared_user])
Copy link
Member

Choose a reason for hiding this comment

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

This can be .add if you don't want to have to make it a list

Copy link
Member

@cabarnes cabarnes left a comment

Choose a reason for hiding this comment

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

Good catch!

@cabarnes cabarnes merged commit c9109a8 into alpha Aug 11, 2019
@cabarnes cabarnes deleted the allow-shared-ws-connect branch August 11, 2019 20:18
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