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

Raise an exception on failed auth #27

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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