Skip to content

Conversation

@vjousse
Copy link
Contributor

@vjousse vjousse commented Jun 15, 2021

@jwoertink during #26 we forgot to reject the connection on a failed auth

I'm using raise UnathorizedConnectionException.new instead of reject_unauthorized_connection just because I find it more explicit.

Copy link
Collaborator

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

Hmm... I'm not doing anything like this in my app. We're just returning nil, but I guess if we're going to add the case for it, then we should use the built-in method reject_unauthorized_connection https://github.com/cable-cr/cable/blob/master/src/cable/connection.cr#L93-L95

This is probably just getting more in to semantics of coding style though... We should probably just focus on adding a lot more documentation on what all is possible. How about instead of doing this, I can look in to getting API docs setup and pushed to github pages? Then we would be able to see everything documented.

Or, actually, maybe we can just add some crystal comments in this method saying something like "you can also raise Connection::UnathorizedConnectionException.new in the case of unauthorized connections"?

@vjousse
Copy link
Contributor Author

vjousse commented Jun 21, 2021

I would go with the latter, adding some Crystal comments. What do you think of

    def connect

      UserToken.decode_user_id(token.to_s).try do |user_id|
        self.identifier = user_id.to_s
        self.current_user =  UserQuery.find(user_id)
      end

      # If you were unable to authenticate the user, you could raise an
      # unauthorized exception, something like:
      # raise UnathorizedConnectionException.new unless UserToken.decode_user_id(token.to_s) …
      #
      # or using the builtin reject_unauthorized_connection method
      #
      # reject_unauthorized_connection unless UserToken.decode_user_id(token.to_s).try …

    end

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.

2 participants