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

Tighter Crossbar topic/rpc ACLs #30

Open
Tracked by #3
frankwiles opened this issue Aug 4, 2020 · 12 comments
Open
Tracked by #3

Tighter Crossbar topic/rpc ACLs #30

frankwiles opened this issue Aug 4, 2020 · 12 comments
Assignees
Labels
enhancement New feature or request question Further information is requested

Comments

@frankwiles
Copy link
Contributor

frankwiles commented Aug 4, 2020

We've discovered a small security issue with how we only check for authenticated users when allowing pub/sub and rpc calls and not whether or not that particular topic or call makes sense for that particular user.

I've got a client who is offering to fund the fix and development of more granular controls. I'm opening this ticket to start the discussion and planning with the rest of the team.

For RPC calls I think the easiest thing to do is:

  1. Provide a @register_leader_only decorator which denies access for any non-leader logins. This would cover many of the issues with most users.
  2. Ensure and document that the user of simpl-modelservice can easily get to the simpl-user information for the calling user (to allow them to limit down themselves even further).

When it comes to players however, I think it gets a bit more complicated. Do we just "enforce" the data model? i.e. I'm not allowed to subscribe to Runs or Worlds I'm not a part of in the API.

Or do we provide a real ACL like system going forward? I think first stab is just a "enforce or don't enforce" the data model settings option but if anyone has any brilliant ideas I'm listening!

@frankwiles frankwiles added enhancement New feature or request question Further information is requested labels Aug 4, 2020
@frankwiles
Copy link
Contributor Author

@joelee484 @janeeisenstein @fcurella Thoughts?

@ghost
Copy link

ghost commented Aug 4, 2020

I like enforcing the data model's access constraints as currently documented and implemented in the Simpl code. In particular, only leaders should be able to subscribe to runs. Once that’s in place, preventing players from subscribing to worlds they’re not in becomes less urgent , but should still be done.

With the addition of game level RPC's for wharton-simpl-admin, I'd like to be able to limit their access to admin users. Would that be covered by your second proposal?

@fcurella
Copy link
Contributor

fcurella commented Aug 4, 2020

Could we leverage something like django-guardian and provide API views on simpl-games-api for model services to check for permissions? Basicall pass a user id, content-type and object id and it returns True/False.

Then leave it to the specific game implementation to do whatever they want with that info. In addition, we could also provide some convenience functions or decorator for common scenarios like @only_leader or @only_owner

@frankwiles
Copy link
Contributor Author

@janeeisenstein "admin" level RPC calls makes sense to me, I'll add that into the mix.

@frankwiles
Copy link
Contributor Author

@fcurella wouldn't repeated calls back to the API for things like Decisions/Results get cumbersome?

@ghost
Copy link

ghost commented Aug 7, 2020

@frankwiles Currently, an RPC user is required to be registered as either a leader or player of the game -- except when the RPC is defined at the game level. I'm not sure that's a bad thing, but it is a thing.

@ghost ghost closed this as completed Aug 7, 2020
@ghost ghost reopened this Aug 7, 2020
@fcurella
Copy link
Contributor

Assuming that row-level permission is a pretty common scenario (use-case: players can only access a specific run), at some point the permission needs to be persisted in the API service in order to survive a restart of the model service.

Querying every time would be cumbersome in terms of network chattiness, but I think we can alleviate that with some light caching on the model service.

As for 'cumbersome' from an implementor perspective (ie: coding a sim), that's why we should write a set of decorators or functions for at least the most common cases.

@ghost
Copy link

ghost commented Aug 12, 2020

@fcurella At Wharton, we currently limit players to being able to access one active run at a time. Players actually have free access to at most one world as well as their private scenarios. This is supported using the current simpl-modelservice and default frontend view code. Please explain how an individual player might attempt to access other runs.

@fcurella
Copy link
Contributor

@janeeisenstein using runs was a wrong example on my part. Runs are already related to Users in simpl-games-api via the RunUser model.

IIRC a Decision is not strictly related to a RunUser, so in theory any user could see decisions from users of the same run, unless the the modelservice implements custom logic that ties the decisions to the user and enforces it. Maybe all we need is to abstract that logic for the most common use-cases to provide re-usability for sim implementors.

@ghost
Copy link

ghost commented Aug 12, 2020

Decisions are associated with roles rather than users. That said, there are real world use cases for hiding some of the world information that is currently loaded into a player's browser. Currently, all of a world's decisions and results are shared with all players in the world. For competitive games, it makes sense to support only loading decisions and results created with the current player's role. Currently, all the runusers in the player's world are loaded into the browser. There is a use case to support hiding runuser information of a world's players from each other for privacy reasons. @frankwiles what use cases were you given for implementing this feature? I'd like to know whether hiding currently shared data belongs to this issue or should go into separate issues.

@ghost
Copy link

ghost commented Aug 12, 2020

@fcurella Filtering occurs in the simpl-modelservice to ensure users access only data that is appropriate for them. Perhaps review the current simpl-modelservice and simpl-react code and see where a user might gain access to inappropriate data via WAMP?

@ghost
Copy link

ghost commented Aug 12, 2020

@frankwiles only leaders/admins should be able to invoke get_active_runusers, invoke advance_phase and rollback_phase. Also, does get_scope_tree need access control or is current filtering sufficient? Islist_scopes used? If not, let's retire it.

@frankwiles frankwiles self-assigned this Oct 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants