Crivaledaz / Mattermost-LDAP

This module provides an external LDAP authentication in Mattermost for the Team Edition (free).
MIT License
359 stars 71 forks source link

Bug in Mattermost or compatibility issue with Mattermost-LDAP? #9

Closed wapsi closed 6 years ago

wapsi commented 7 years ago

I almost got this to work but after I hit "Authorize" button (authorize.php) and the browser goes back to the Mattermost (URL https://foo.bar/signup/gitlab/complete?code=XXX=YYY) I'll get 500 HTTP error. And here is what happens at the Mattermost side:

`[2017/09/04 13:07:15 EEST] [EROR] Please check the std error output for the stack trace [2017/09/04 13:07:15 EEST] [EROR] [runtime error: invalid memory address or nil pointer dereference] goroutine 2569 [running]: runtime/debug.Stack(0x164ebe0, 0xc42135c6e0, 0x10e8343) /usr/local/go/src/runtime/debug/stack.go:24 +0x79 runtime/debug.PrintStack() /usr/local/go/src/runtime/debug/stack.go:16 +0x22 github.com/mattermost/platform/vendor/github.com/gorilla/handlers.recoveryHandler.log(0x164ece0, 0xc421032958, 0x164ed20, 0x16c69d0, 0x1, 0xc42135c680, 0x1, 0x1) /var/lib/jenkins/jobs/msr/jobs/d/jobs/platform-release/workspace/src/github.com/mattermost/platform/vendor/github.com/gorilla/handlers/recovery.go:89 +0x70 github.com/mattermost/platform/vendor/github.com/gorilla/handlers.recoveryHandler.ServeHTTP.func1(0x1657ce0, 0xc421facd20, 0x164ece0, 0xc421032958, 0x164ed20, 0x16c69d0, 0x1) /var/lib/jenkins/jobs/msr/jobs/d/jobs/platform-release/workspace/src/github.com/mattermost/platform/vendor/github.com/gorilla/handlers/recovery.go:74 +0xef panic(0xfb1000, 0x1690640) /usr/local/go/src/runtime/panic.go:489 +0x2cf github.com/mattermost/platform/model/gitlab.(GitLabProvider).GetAuthDataFromJson(0x16c69d0, 0x164e5e0, 0xc4220f2a50, 0x6, 0xc420c7e508) /var/lib/jenkins/jobs/msr/jobs/d/jobs/platform-release/workspace/src/github.com/mattermost/platform/model/gitlab/gitlab.go:109 +0x3e github.com/mattermost/platform/app.LoginByOAuth(0xc421facb6c, 0x6, 0x7f63142a1028, 0xc422eb8120, 0x0, 0x0, 0x0, 0x0) /var/lib/jenkins/jobs/msr/jobs/d/jobs/platform-release/workspace/src/github.com/mattermost/platform/app/oauth.go:450 +0x203 github.com/mattermost/platform/app.CompleteOAuth(0xc421facb6c, 0x6, 0x1654aa0, 0xc422eb8120, 0x0, 0x0, 0xc4220b05d0, 0x0, 0x0) /var/lib/jenkins/jobs/msr/jobs/d/jobs/platform-release/workspace/src/github.com/mattermost/platform/app/oauth.go:430 +0x2d4 github.com/mattermost/platform/api4.completeOAuth(0xc4217eda40, 0x1657ce0, 0xc421facd20, 0xc422649e00) /var/lib/jenkins/jobs/msr/jobs/d/jobs/platform-release/workspace/src/github.com/mattermost/platform/api4/oauth.go:423 +0x83d github.com/mattermost/platform/api4.handler.ServeHTTP(0x110e388, 0xc421000000, 0x1657ce0, 0xc421facd20, 0xc422649e00) /var/lib/jenkins/jobs/msr/jobs/d/jobs/platform-release/workspace/src/github.com/mattermost/platform/api4/context.go:169 +0x9c5 github.com/mattermost/platform/api4.(handler).ServeHTTP(0xc4228c9830, 0x1657ce0, 0xc421facd20, 0xc422649e00)

:1 +0x83 github.com/mattermost/platform/vendor/github.com/gorilla/mux.(*Router).ServeHTTP(0xc4223b5d60, 0x1657ce0, 0xc421facd20, 0xc422649e00) /var/lib/jenkins/jobs/msr/jobs/d/jobs/platform-release/workspace/src/github.com/mattermost/platform/vendor/github.com/gorilla/mux/mux.go:114 +0x10c github.com/mattermost/platform/app.(*CorsWrapper).ServeHTTP(0xc421032958, 0x1657ce0, 0xc421facd20, 0xc422649c00) /var/lib/jenkins/jobs/msr/jobs/d/jobs/platform-release/workspace/src/github.com/mattermost/platform/app/server.go:75 +0x78 github.com/mattermost/platform/vendor/github.com/gorilla/handlers.recoveryHandler.ServeHTTP(0x164ece0, 0xc421032958, 0x164ed20, 0x16c69d0, 0x1, 0x1657ce0, 0xc421facd20, 0xc422649c00) /var/lib/jenkins/jobs/msr/jobs/d/jobs/platform-release/workspace/src/github.com/mattermost/platform/vendor/github.com/gorilla/handlers/recovery.go:78 +0xb9 github.com/mattermost/platform/vendor/github.com/gorilla/handlers.(*recoveryHandler).ServeHTTP(0xc42187d170, 0x1657ce0, 0xc421facd20, 0xc422649c00) :59 +0x87 net/http.serverHandler.ServeHTTP(0xc4217c14a0, 0x1657ce0, 0xc421facd20, 0xc422649c00) /usr/local/go/src/net/http/server.go:2568 +0x92 net/http.(*conn).serve(0xc42142dd60, 0x1658660, 0xc422ea6340) /usr/local/go/src/net/http/server.go:1825 +0x612 created by net/http.(*Server).Serve /usr/local/go/src/net/http/server.go:2668 +0x2ce ` Does anyone have a clue is this bug in Mattermost or in Mattermost-LDAP OAuth "addon"? How can I debug this issue more in detail? I tried with Mattermost 4.1.0 Community/Team & Enterprise editions: both will fail with that same error. And I've configured Nginx reverse proxy to provide HTTPS before Mattermost. BTW: I tested the LDAP auth using the command @Crivaledaz provided here: https://github.com/Crivaledaz/Mattermost-LDAP/issues/5 (curl --header "Authorization: Bearer ACCESS_TOKEN" http://OAUTH_SERVER/oauth/resource.php) and it returned all necessary fields just fine.
steve-oakey commented 7 years ago

We had the same problem. I think the JSON returned from resource.php is incorrect for what Mattermost is trying to decode (at least for Mattermost 4.x). I cannot submit a PR at the moment, but in [resource.php](https://github.com/Crivaledaz/Mattermost-LDAP/blob/master/oauth/resource.php#L36) we changed:

$resp = array("name" => $data['cn'],"username" => $user,"id" => $assoc_id,"state" => "active","email" => $data['mail']);

to

$resp = array("name" => $data['cn'],"username" => $user,"id" => (int)$assoc_id,"state" => "active","email" => $data['mail'],"login" => $user);

And it worked. I think the problems were (according to this struct in the Mattermost server)

  1. id was expecting a number
  2. login was missing.
Crivaledaz commented 7 years ago

Hi,

I am using Mattermost 4.1 and I have not any problem. Maybe the problem come from the version 4.2. I will test your suggestion to check if this break anything for me.

Concerning the id, I can confirm is an integer. Normally, to fill this field, Mattermost-LDAP create a new id each time a new user sign in the oauth server. This number is stored in the users table. Maybe the variable need to be cast as you suggest.

However, I never heard about the login field. I will find out, but if this works with the username, that is cool :).

Thank you for your feedbacks and your suggestion, you possibly solve the issue #5 at the same time.

Crivaledaz

steve-oakey commented 7 years ago

@Crivaledaz - Yes, I can confirm this fix is for Mattermost 4.2 specifically as this is what we are using. I am not sure if the same fix would work for Mattermost 4.1 or earlier but looking at the Mattermost source code for earlier versions, it looks like the struct hasn't changed (since at least 4.0.0)

type GitLabUser struct {
    Id       int64  `json:"id"`
    Username string `json:"username"`
    Login    string `json:"login"`
    Email    string `json:"email"`
    Name     string `json:"name"`
}

The stack trace above (which is similar to the one we had) wasn't descriptive enough to tell me what value was incorrect and I am not familiar with how lenient the Golang JSON parsing is (if it will cast a string to an int64 automatically, for example) so I just tried to make the user JSON response match this struct exactly.

Crivaledaz commented 6 years ago

Hi,

I think this problem is solved with issue #20. In fact, Mattermost-LDAP was not consistent anymore with Mattermost, because Gitlab have modified the json data sent after authorization process. Normally, the new version of Mattermost-LDAP should solve your issue too.

@steve-oakey In fact, the struct does not match with the JSON returned by Gitlab. Actually, there is no login value in the data returned by Gitlab. However, Mattermost 4.9 need all the data sent by Gitlab, even if it uses only the 4 values. Without all the data Mattermost raise the stack error describe in this issue.

I hope the patch will fix your problem, let me know if you encounter other problems with Mattermost-LDAP.