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

Session.Open() should validate authentication #198

Closed
le-jzr opened this issue May 11, 2016 · 9 comments
Closed

Session.Open() should validate authentication #198

le-jzr opened this issue May 11, 2016 · 9 comments
Assignees
Labels
feature Major feature implementation
Milestone

Comments

@le-jzr
Copy link

le-jzr commented May 11, 2016

When an application authenticates using just a token, it is impossible to tell whether the connection succeeded or not. In fact, providing a garbage token doesn't seem to make any difference at all, when trying to test a simple bot.

@le-jzr le-jzr changed the title Session.Open() should validate token Session.Open() should validate authentication May 11, 2016
@IMcPwn
Copy link

IMcPwn commented May 12, 2016

Currently I use the follow snippet to check if the login was successful.

_, err := dg.User("@me")
    if err != nil {
        // Login unsuccessful
        fmt.Println(err)
        return
    }
   // Login successful

@bwmarrin
Copy link
Owner

I've thought about this for a bit and decided I don't really want to add this type of code into the library itself because my impression is that 99% of the time people should be using valid user names, passwords, tokens, etc. So adding extra checks should be unnecessary code - which I'd like to avoid if possible.

I'll look into better reporting down the road of the websocket connection though. Once the identify packet is sent over the websocket there should be some response saying it's a bad token that I can give an error or event for.

@bwmarrin bwmarrin added this to the v0.13.0 milestone May 17, 2016
@bwmarrin bwmarrin added the feature Major feature implementation label May 17, 2016
@le-jzr
Copy link
Author

le-jzr commented May 17, 2016

99% of the time people should be using valid user names, passwords, tokens, etc.

Although I don't see any immediate harm in this particular case (short of making the library less intuitive for people who are just starting to use it), the reasoning given above is something any programmer should be terrified of. The cornerstone of any software engineering is assuming that the users will provide your code with literally the worst input possible. If you don't assume that, security disasters are only a matter of time.

@b1naryth1ef
Copy link
Contributor

@Zarevucky although thats true, in this case you should not being planning to treat developers like unskilled children. As a developer, if I use a library for the first time and it doesn't immediately work as I assume, my thought would be to blame my implementation vs the library itself.

@bwmarrin the best way to achieve this w/o an additional request is just to listen to the WS error codes, and handle 4 aka 'authentication_failed'.

@le-jzr
Copy link
Author

le-jzr commented May 17, 2016

@b1naryth1ef All I mean is that any decision based on assumption about input correctness must be made with utmost care, much more than any other kind of argument. I have utmost respect towards the work made by @bwmarrin and others, and I would not make any statement regarding skill or maturity. I simply do not assume every developer comes from the same background as I do.

@bwmarrin
Copy link
Owner

So, the 2nd half of my response up there was to mean, what @b1naryth1ef mentioned. I'll get a websocket response for a bad token and I'll change the lib to do something more obvious with that.

The goal of discordgo is actually to provide a very low level direct mapping of Discord API with as little "extra fluff" as possible. So handing the Open the exact same way Discord handles it is what I'd opt for.

Now, if I was writing a bot or a custom client and I was going to accept login info from end-users that run and use my tool. I'd probably have a different approach 😉

@bwmarrin bwmarrin removed this from the v0.13.0 milestone Jul 22, 2016
@bwmarrin bwmarrin added this to the v0.17.0 milestone Apr 18, 2017
@bwmarrin bwmarrin modified the milestones: v0.18.0, v0.17.0 Sep 5, 2017
@Hinara
Copy link
Contributor

Hinara commented Sep 30, 2017

Can we use GET/gateway/bot ?
This will verify that we have a valid bot token, however this will probably drop the support of normal users but this will be pretty useful as it can return the number of shards to have too.

@bwmarrin bwmarrin self-assigned this Nov 10, 2017
bwmarrin added a commit that referenced this issue Nov 11, 2017
Now the open function will follow through a bit more and insure that the
proper sequence of events happens during the Open call.  This required
some refactoring and a few mild changes in the onEvent func.
@bwmarrin
Copy link
Owner

bwmarrin commented Nov 11, 2017

It took awhile.. :(

But, if you call Open using an invalid token it will return an error telling you so. This happens by listening for and returning the 4004 error sent by Discord. It will also return any other errors Discord happens to return too :)

If Open doesn't return an error - you should have a functional connection.

@Hinara
Copy link
Contributor

Hinara commented Dec 24, 2017

Great 😄

ErikMcClure pushed a commit to ErikMcClure/discordgo that referenced this issue Aug 4, 2020
Now the open function will follow through a bit more and insure that the
proper sequence of events happens during the Open call.  This required
some refactoring and a few mild changes in the onEvent func.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Major feature implementation
Projects
None yet
Development

No branches or pull requests

5 participants