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 lock to PerspectiveManager #999

Merged
merged 2 commits into from
Apr 6, 2020
Merged

Add lock to PerspectiveManager #999

merged 2 commits into from
Apr 6, 2020

Conversation

sc1f
Copy link
Contributor

@sc1f sc1f commented Apr 3, 2020

This PR adds the lock attribute to PerspectiveManager, which will reject the following remote messages that can mutate the state of Tables and Views under management:

  • table
  • table.update
  • table.clear
  • table.replace
  • table.reset
  • table.delete

PerspectiveManagers exposed over the internet should be locked, either by calling .lock() or initializing with lock=True. This PR also includes tests and a compact example demonstrating message rejection when the manager is locked.

@sc1f sc1f added internal Internal refactoring and code quality improvement Python labels Apr 3, 2020
@sc1f sc1f requested a review from timkpaine April 3, 2020 19:37
@timkpaine
Copy link
Member

I understand the concept but if we want to prevent js client modifications, this should be a construction time attribute that cannot be undone (e.g. no unlock). As is this current PR feels like "authentication-light" which is not something we want to be doing.

@sc1f
Copy link
Contributor Author

sc1f commented Apr 3, 2020

I understand the concept but if we want to prevent js client modifications, this should be a construction time attribute that cannot be undone (e.g. no unlock). As is this current PR feels like "authentication-light" which is not something we want to be doing.

Agreed - wondering if we should just default the manager to locked as well

@timkpaine
Copy link
Member

Agreed - wondering if we should just default the manager to locked as well

I think so.

The followup question is do you want to implement a token-granting system for perspective? This would be just on the client-server api, basically the ability to:

  • restrict read/write access (separately) using a token
  • ask the server for a new token
  • ask the server to revoke a token
  • tell the server about existing tokens, how to auth them

Then web servers could either provide a token scheme, or otherwise put the token granting behind an authenticated api (using their own authentication). This keeps us out of the authentication business while still allowing perspective to work nicely in authenticated contexts.

@sc1f
Copy link
Contributor Author

sc1f commented Apr 3, 2020

Yeah this is definitely working up to a token-based authentication scheme for the remote API.

Adding a lock is mostly for being able to host python-based perspective remote examples on the internet without having to worry about getting the example table cleared/deleted remotely.

@texodus texodus requested review from texodus and removed request for timkpaine April 6, 2020 06:11
under management.

All `PerspectiveManager`s exposed over the internet should be locked to
prevent content from being mutated by clients.
Copy link
Member

Choose a reason for hiding this comment

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

This language simplifies things a bit :)

Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Changes look good!

Perspective is not currently scoped for granular, secure, full duplex communications to a virtual host. The intention of this feature is to simply allow read-only mode for documented APIs that are vandalism-prone for use in kiosk/demo mode, after whatever necessary mutable setup is done.

@texodus texodus merged commit e0cf84c into master Apr 6, 2020
@texodus texodus deleted the tornado-lock branch January 18, 2021 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Internal refactoring and code quality improvement Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants