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

Tethys Async Websocket Consumer with Permission Checks #1012

Merged
merged 18 commits into from
Mar 12, 2024

Conversation

ckrew
Copy link
Contributor

@ckrew ckrew commented Feb 20, 2024

This PR is to resolve issue #1009.

The newly created TethysAsyncWebsocketConsumer class will replace the AsyncWebsocketConsumer class. The new TethysAsyncWebsocketConsumer class has the following capabilities:

  • Developers can provide a list of permissions to the class for user access
  • When attempting to connect, the class will check the user permissions to ensure access is granted. If authorized, the user will be connected. If not authorized, the websocket will close with a 4004 error.
  • Developers can use the custom on_connect, on_authorized_connect, on_receive, and on_disconnect class methods to run custom application code
  • The connect, disconnect, and receive methods have been stubbed out for future base development and will run the custom methods described above.

The websocket tutorial docs have been updated as well to show the new capabilities.

@coveralls
Copy link

coveralls commented Feb 21, 2024

Coverage Status

coverage: 100.0%. remained the same
when pulling e20223f on ckrew:websocket_permissions
into 9f5b60c on tethysplatform:main.

@ckrew
Copy link
Contributor Author

ckrew commented Feb 26, 2024

I updated this PR after reviewing some feedback. The main thing was keeping user experience consistent with regards to decorators. Users can now use the new capabilities just like a controller by supplying arguments in the decorator. An example is below:

image

The methods for connecting and disconnecting have now been separated according to user authorization. So there is now a "authorized_connect", "unauthorized_connect", "authorized_disconnect", and "unauthorized_disconnect" so that developers can handle whatever use case they experience.

For the backend, the solution was using a mixin and dynamically adding new methods from the mixin to the decorated class. This works for both async and sync websockets as well.

Copy link
Member

@sdc50 sdc50 left a comment

Choose a reason for hiding this comment

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

I love the updated approach! Excited for this to get merged. I just request one minor change.

"""
base_class_name = inspect.getmro(function_or_class)[1].__name__.lower()
function_or_class_name = function_or_class.__name__.lower()
if "async" in function_or_class_name or "async" in base_class_name:
Copy link
Member

Choose a reason for hiding this comment

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

While it is not very likely to be a problem, there is a small chance that checking for "async" in the function_or_class_name could be miss-leading. Suppose someone were to create a function called: do_something_in_a_non_async_way that was not an async function?

I think it might be safer to just check all of the base classes:

 async_base_classes = [cls for cls in inspect.getmro(function_or_class)[1:]. if "async" in cls.__name__.lower()]
if async_base_classes:
    ...

Copy link
Member

Choose a reason for hiding this comment

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

I suppose this approach isn't foolproof either. Someone could do something like:

from channels.consumer import SyncConsumer, AsyncConsumer

class AnAsyncConsumer(AsyncConsumer):
   pass

class ASyncConsumer(SyncConsumer):
   pass

You never know! 🤷

According to the Tethys Docs:

The consumer decorator functions largely the same way as the controller decorator except that it is used to decorate a consumer class, which must be a subclass of either channels.consumer.AsyncConsumer or channels.consumer.SyncConsumer

I'm hoping that I was smarter when I wrote that than I am now. If it is indeed true that a consumer class must be a subclass of channels.consumer.AsyncConsumer then:

if issubclass(function_or_class, channels.consumer.AsyncConsumer):
    consumer_mixin = TethysAsyncWebsocketConsumerMixin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Scott. Those are valid points. I like the final proposed solution. Simple and clean for sure.

Copy link
Contributor Author

@ckrew ckrew Mar 7, 2024

Choose a reason for hiding this comment

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

Although, maybe that won't work. I just looked at my implementation and I am using the
AsyncWebsocketConsumer class from channels.generic.websocket. That is also what is suggested in the tutorials for the websockets.

Copy link
Member

Choose a reason for hiding this comment

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

The AsyncWebsocketConsumer is a subclass of AsyncConsumer. I think the issubclass will look at the whole parent chain, but I'm not sure.

Copy link
Member

Choose a reason for hiding this comment

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

from channels.generic.websocket import AsyncWebsocketConsumer
class MyConsumer(AsyncWebsocketConsumer):
    groups = ["broadcast"]
    
from channels.consumer import AsyncConsumer
issubclass(MyConsumer, AsyncConsumer)
True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just implemented this. I had to do it the opposite way and checked if it is a subclass of SyncConsumer because the WebsocketConsumer and AsyncWebsocketConsumer are both subclasses of AsyncConsumer apparently.

@sdc50
Copy link
Member

sdc50 commented Mar 6, 2024

@swainn It looks like the Docker build is failing because the secrets.DOCKER_UPLOAD_URL is not being substituted in correctly. I'm not sure why that would be the case. On successful jobs it looks like this: ***:dev, but in this one its just :dev.

@sdc50
Copy link
Member

sdc50 commented Mar 12, 2024

I think we decided that the docker build won't work because this is being merged from a fork. Once other checks pass then I approve merging.

@sdc50 sdc50 merged commit d65dd68 into tethysplatform:main Mar 12, 2024
9 of 10 checks passed
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