Add support of custom oauth2 server (issue#11) #29

nbys · 2019-07-28T09:52:27Z

To resolve #11

I used dev_provider as example for this implementation. Basically I just replaced hardcoded parts of dev_provider with corresponding methods of go-oauth2/oauth2.

… same as domain of redir url

nbys

[DELETED]

coveralls · 2019-07-28T10:48:41Z

Pull Request Test Coverage Report for Build 250

151 of 182 (82.97%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.5%) to 85.418%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
provider/custom_server.go	111	142	78.17%

Totals
Change from base Build 240:	-0.5%
Covered Lines:	1482
Relevant Lines:	1735

💛 - Coveralls

nbys · 2019-07-28T15:48:24Z

provider/oauth2.go

@@ -161,6 +161,8 @@ func (p Oauth2Handler) AuthHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}

+	p.conf.RedirectURL = p.makeRedirURL(r.URL.Path)


go-oauth2/oauth2 does check of redir url also during token exchange

umputun

This is impressive work, thx a lot. I didn't play with the code yet (going to do it later on, hopefully, today) and just noticed a few relatively minor things from the first sight.

umputun · 2019-07-28T17:52:11Z

provider/custom_server.go

+func (c *CustomOauthServer) handleAuthorize(w http.ResponseWriter, r *http.Request) {
+	// Called for first time, ask for username
+	if c.WithLoginPage && (r.ParseForm() != nil || r.Form.Get("username") == "") {
+		userLoginTmpl, err := template.New("page").Parse(customLoginTmpl)


The customLoginTmpl is fine as a default template but I'd suggest adding user-defined/injected one to CustomProviderOpt

I added function LoginPageHandler to allow user to handle login page on its own. Usage example in here _example/custom/main.go
Let me know if you prefer more stricter way to let pass just a template.

umputun · 2019-07-28T17:55:32Z

provider/custom_server.go

+	}
+	userID := ti.GetUserID()
+
+	ava := fmt.Sprintf(c.Domain+":%d/avatar?user=%s", custOAuthPort, userID)


I'm not sure yet how all these flows works (just spent a few minutes with this PR) but generally speaking making URL with "naked" sprintf makes me worry a little bit. Maybe we better sanitize the potentially vulnerable URL like this ?

umputun · 2019-07-28T18:00:33Z

provider/custom_server.go

+func (c *CustomOauthServer) handleAuthorize(w http.ResponseWriter, r *http.Request) {
+	// Called for first time, ask for username
+	if c.WithLoginPage && (r.ParseForm() != nil || r.Form.Get("username") == "") {
+		userLoginTmpl, err := template.New("page").Parse(customLoginTmpl)


Test coverage for this file is below 70% and this branch of code (within if c.WithLoginPage && ...) not covered at all.

Not sure if you can see coveralls report, but in case if it open publicly see https://coveralls.io/builds/24836939/source?filename=provider%2Fcustom_server.go#L80

Yes, I can see coveralls report. I had to check test coverage before submitting pull request. sorry. I'll take care of it.

umputun · 2019-08-02T05:21:46Z

the build failed due to lint issue. They are probably minor, but will be nice to get them addressed:

auth.go:318:9: nilness: impossible condition: nil != nil (govet)
980	if err != nil {
981	       ^
982provider/custom_server.go:67:5: shadow: declaration of "err" shadows declaration at line 44 (govet)
983				err := c.OauthServer.HandleAuthorizeRequest(w, r)
984				^
985provider/custom_server.go:73:5: shadow: declaration of "err" shadows declaration at line 44 (govet)
986				err := c.OauthServer.HandleTokenRequest(w, r)
987				^
988provider/custom_server.go:78:9: shadow: declaration of "err" shadows declaration at line 44 (govet)
989				ti, err := c.OauthServer.ValidationBearerToken(r)
990				    ^
991provider/custom_server.go:147:25: `retrive` is a misspelling of `retrieve` (misspell)
992		p.Logf("[ERROR] can't retrive domain from service URL %s", p.URL)
993		                      ^
994auth.go:298:32: `retrive` is a misspelling of `retrieve` (misspell)
995		return fmt.Errorf(`failed to retrive client from custom ouath server 
996		                             ^
997auth.go:303:45: `diffrent` is a misspelling of `different` (misspell)
998		s.logger.Logf("[WARN] client domain=%s is diffrent from service root url=%s", client.GetDomain(), s.opts.URL)
999		                                          ^
1000auth.go:327:32: `retrive` is a misspelling of `retrieve` (misspell)
1001		s.logger.Logf("[ERROR] can't retrive domain from service URL %s", s.opts.URL)
1002		                             ^
1003provider/custom_server.go:41:1: cyclomatic complexity 17 of func `(*CustomOauthServer).Run` is high (> 15) (gocyclo)
1004func (c *CustomOauthServer) Run(ctx context.Context) {
1005^
1006provider/custom_server.go:80:36: ST1013: should use constant http.StatusUnauthorized instead of numeric literal 401 (stylecheck)
1007					http.Error(w, http.StatusText(401), 401)

nbys · 2019-08-02T06:00:24Z

@umputun Lint errors you mentioned are fixed with d22b9ef and bea3585

umputun · 2019-08-02T06:08:19Z

oops, sorry my bad. It is 1am here, and after a day of heavy codding reading inversed order of commits got me confused.

umputun · 2019-08-02T06:30:12Z

A question about _example/custom. It looks like you work inside of the GOPATH and, unless I miss something, this example is not "go mod" friendly. Could you add go.mod and go.sum pls, i.e. "go mod init" + "go get"? Probably you will need to use the replace for github.com/go-pkgz/auth, in the same way as _example/go.mod does.

umputun · 2019-08-02T06:57:45Z

Another suggestion regarding the example. Currently, it is a little bit confusing for someone running it as go run main.go. After hitting http://127.0.0.1:8080/auth/custom/login it gets prompt and with admin/admin authenticates just fine. However attempt to hit protected URL in the browser, i.e. http://127.0.0.1:8080/private still returns "Unauthorized" due to understandable xsrf mismatch. The example doesn't print much either (opts.Logger not assigned), so it can be not easy to figure what's going on.

I'd suggest moving the custom provider example to top-level (example/main.go). This way, we will get it nicely integrated and easily demonstratable as it will be shown as one more provider in addition to others.

umputun · 2019-08-03T18:21:58Z

A few things:

The example missing dependencies in go.mod/go.sum. In order to add them you can do go get. If you work inside of GOPATH it will be GO111MODULE=on go get
As you used dev_provider,which is special/dev thingy, as an example, the CustomProvider doesn't follow the usual logic all normal providers implement, i.e. it is not added to service explicitly with service.AddCustomProvider
Generally speaking, custom provider exposes too many implementation details to the consumer. I mean, the only goal to have auth.go is to help the user with the wiring of all things together and hide (enclose) dependency initialization. For example, it doesn't expose jwtService but provides high-level wrapper as a part of auth.NewService. The same level of abstraction needed for the custom provider as well. Ideally, the user should fill CustomProviderOpt and pass it to service.AddCustomProvider("custom123", copts) and it will do the job.

nbys · 2019-08-03T18:47:19Z

I got it. Thank you for your remarks.
sorry for go mod. I forgot again that _example/main.go has its own go.mod

nbys · 2019-08-05T21:10:50Z

As you suggested I added the method to register custom provider in handler stack: service.AddCustomProvider("custom123", copts)
Where copts are endpoint, infoURL and mapUserFn. The idea is allow to add the custom provider running on local as well as on different host. As we wouldn’t sure about implementation of mapUserFn on server side I allowed to inject one.

After I removed this part from auth.go it is now responsibility of library user to initiate and start custom server. I added a helper method to initiate server instance and prefill some options needed later for AddCustomProvider. Tbh Im not sure about this. There are obvious things in this method but maybe you’ll find it useful.

I changed a bit the names as well trying to differentiate the process of registering provider from starting oauth server.

umputun · 2019-08-10T18:23:36Z

A few suggestions and observations. Pls note, I'm not 100% sure those are good suggestions, just smth to think about:

NewCustomServer(srv *goauth2.Server, sopts CustomServerOpts) (*CustomServer, CustomProviderOpt) - accepting options and returning another option(s) from the constructor, which is highly unusual. Can CustomProviderOpt be encapsulated as a private field in CustomServer?
In case if it can, NewCustHandler could be converted to a method of CustomServer and renamed to CustHandler or even Handler.
Maybe AddCustomProvider(name string, copts provider.CustomProviderOpt) wrapper should accept CustomServer instead of CustomProviderOpt?

nbys · 2019-08-11T12:07:55Z

@umputun
1,2 are valid points. I have to refactor the code to look more usual and understandable.

Regarding 3:
I just try to clarify what I tried to achieve.
I wanted this method is to be more general. I'd like AddCustomProvider be able to register not only go-oauth2/oauth2 provider but maybe some different Oauth2 provider. For example, currently we don't have an implementation of bitbucket login in go-pkgz/auth. But library user would able to add bitbucket login on its own:

service.AddCustomProvider("bitbucket", provider.CustomProviderOpt{
	Endpoint: oauth2.Endpoint{
		AuthURL:  "https://bitbucket.org/site/oauth2/authorize",
		TokenURL: "https://bitbucket.org/site/oauth2/access_token",
	},
	InfoURL: "https://api.bitbucket.org/2.0/user/",
	Cid:     "cid",
	Csecret: "csecret",
	MapUserFn: func(data provider.UserData, _ []byte) token.User {
		userInfo := token.User{
			ID:   "bitbucket_" + token.HashID(sha1.New(), 
                                data.Value("username")),
			Name: data.Value("nickname"),
		}
		return userInfo
	},
	Scopes: []string{"account"},
	})

Note: it is not fully testable right now. I have to implement some changes (e.g. make UserData type public). But locally I played a bit with it and it looks good.
Please let me know what do you think about this idea.

umputun · 2019-08-11T15:51:33Z

be able to register not only go-oauth2/oauth2 provider but maybe some different Oauth2 provider

makes sense. I have missed that use case, but it will be very nice to have.

nbys · 2019-08-13T16:02:13Z

as you suggested I encapsulated handler opts as a field in CustomServer
the method NewCustHandler was renamed NewCustom. The purpose of this method the same as of methods NewYandex, NewFacebook etc.
I added two examples in _example/main.go. one for our self-hosted goauth2/oauth2 server, the other for unknown oauth2 provider (i.e. bitbucket)

It seems Travis checks have failed because coveralls is on maintenance at the moment.

umputun · 2019-08-15T00:36:47Z

Looks good to me. Thx for the amazing PR, appreciate it!

umputun · 2019-08-15T00:56:20Z

I just realized we completely missed any documentation. I have added smth minimal to README, but feel free to extend/modify it.

nbys · 2019-08-15T13:50:02Z

@umputun yes, you are right. I'll be back with a new PR for documentation maybe on the weekend.

Thanks for your advices and remarks. I do not work with Golang on daily basis. it is nice way to learn smth new. Very appreciate it!

umputun · 2022-02-08T22:29:03Z

@nbys due to known security-related issues in go-oauth2/oauth2 I have switched it to v4. The api for v4 is almost identical and the only thing I had to adjust was an extra param to generates.NewJWTAccessGenerate.

However, I'm not sure if it works properly. I get all kind of errors as I try to auth with _example.

Can you pls take a look ?

nbys · 2022-02-09T07:07:49Z

@nbys due to known security-related issues in go-oauth2/oauth2 I have switched it to v4. The api for v4 is almost identical and the only thing I had to adjust was an extra param to generates.NewJWTAccessGenerate.

However, I'm not sure if it works properly. I get all kind of errors as I try to auth with _example.

Can you pls take a look ?

I'll take care of this. To understand, do the errors occur only for CustomProvider?

UPD: I tested it a bit. It seems that the problem occurs only with custom oauth2 server:

auth/_example/main.go

Line 153 in c7d6348

    
           service.AddCustomProvider("custom123", auth.Client{Cid: "cid", Csecret: "csecret"}, prov.HandlerOpt)

The another example of CustomProvider is bitbucket. I tested it and It works just fine:

auth/_example/main.go

Lines 161 to 176 in c7d6348

    
           service.AddCustomProvider("bitbucket", c, provider.CustomHandlerOpt{ 
        
           	Endpoint: oauth2.Endpoint{ 
        
           		AuthURL:  "https://bitbucket.org/site/oauth2/authorize", 
        
           		TokenURL: "https://bitbucket.org/site/oauth2/access_token", 
        
           	}, 
        
           	InfoURL: "https://api.bitbucket.org/2.0/user/", 
        
           	MapUserFn: func(data provider.UserData, _ []byte) token.User { 
        
           		userInfo := token.User{ 
        
           			ID: "bitbucket_" + token.HashID(sha1.New(), 
        
           				data.Value("username")), 
        
           			Name: data.Value("nickname"), 
        
           		} 
        
           		return userInfo 
        
           	}, 
        
           	Scopes: []string{"account"}, 
        
           })

I don't think it is critical, but nevertheless, I'll take a look at the weekend.

nbys added 6 commits July 28, 2019 00:09

custom server for go-oauth2/oauth2

ada2884

add method to get domain from service root URL

04e238b

set redirect url before token exchange

a024364

add service method to start local go-oauth2/oauth2 server

8b1aa78

example to custom provider

687a0ea

add warning message that domain from oauth2 client store shoud be the…

d7d2754

… same as domain of redir url

nbys commented Jul 28, 2019

View reviewed changes

fix misspelling

d22b9ef

nbys changed the title ~~Issue#11~~ Add support of custom oauth2 server (issue#11) Jul 28, 2019

move handlers in separate methods

bea3585

nbys commented Jul 28, 2019

View reviewed changes

umputun reviewed Jul 28, 2019

View reviewed changes

nbys added 7 commits July 29, 2019 21:21

allow user to pass login page template handler

c812c9e

changed name of template

8f4567f

add login page to provider initialization

413d69d

add example of usage user-defined template

8f4479b

correct log messages

8a51e64

add tests

cd259c8

sanitize avatar url

950b10c

move custom provider example to example/main.go

9442d7a

nbys added 2 commits August 5, 2019 23:02

update go.mod

2a88837

add method to register custom provider

4fa38c3

nbys added 3 commits August 5, 2019 23:04

add opts and helper funtion for custom provider

52dbf8b

cleanup added function (not used anymore)

f2d8170

add example for custom provider

7a15053

nbys added 2 commits August 5, 2019 23:30

fix errors after travis check

56ff77b

corrections after golangci-lint check

c4b94aa

nbys added 4 commits August 13, 2019 17:23

make UserData type public

1b84856

refactor custom provider

b87fc93

refactor AddCustomProvider method

4508c08

update example

3f33953

umputun merged commit 9487b42 into go-pkgz:master Aug 15, 2019

nbys mentioned this pull request Feb 10, 2022

fix _example with CustomProvider on go-auth2/oauth2 server #108

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support of custom oauth2 server (issue#11) #29

Add support of custom oauth2 server (issue#11) #29

nbys commented Jul 28, 2019

nbys left a comment •

edited

Loading

coveralls commented Jul 28, 2019 •

edited

Loading

nbys Jul 28, 2019

umputun left a comment

umputun Jul 28, 2019

nbys Jul 31, 2019

umputun Jul 28, 2019

nbys Aug 2, 2019

umputun Jul 28, 2019

nbys Jul 28, 2019 •

edited

Loading

umputun commented Aug 2, 2019

nbys commented Aug 2, 2019 •

edited

Loading

umputun commented Aug 2, 2019

umputun commented Aug 2, 2019

umputun commented Aug 2, 2019

umputun commented Aug 3, 2019

nbys commented Aug 3, 2019

nbys commented Aug 5, 2019 •

edited

Loading

umputun commented Aug 10, 2019 •

edited

Loading

nbys commented Aug 11, 2019

umputun commented Aug 11, 2019

nbys commented Aug 13, 2019 •

edited

Loading

umputun commented Aug 15, 2019

umputun commented Aug 15, 2019 •

edited

Loading

nbys commented Aug 15, 2019

umputun commented Feb 8, 2022

nbys commented Feb 9, 2022 •

edited

Loading

Add support of custom oauth2 server (issue#11) #29

Add support of custom oauth2 server (issue#11) #29

Conversation

nbys commented Jul 28, 2019

nbys left a comment • edited Loading

Choose a reason for hiding this comment

coveralls commented Jul 28, 2019 • edited Loading

Pull Request Test Coverage Report for Build 250

💛 - Coveralls

nbys Jul 28, 2019

Choose a reason for hiding this comment

umputun left a comment

Choose a reason for hiding this comment

umputun Jul 28, 2019

Choose a reason for hiding this comment

nbys Jul 31, 2019

Choose a reason for hiding this comment

umputun Jul 28, 2019

Choose a reason for hiding this comment

nbys Aug 2, 2019

Choose a reason for hiding this comment

umputun Jul 28, 2019

Choose a reason for hiding this comment

nbys Jul 28, 2019 • edited Loading

Choose a reason for hiding this comment

umputun commented Aug 2, 2019

nbys commented Aug 2, 2019 • edited Loading

umputun commented Aug 2, 2019

umputun commented Aug 2, 2019

umputun commented Aug 2, 2019

umputun commented Aug 3, 2019

nbys commented Aug 3, 2019

nbys commented Aug 5, 2019 • edited Loading

umputun commented Aug 10, 2019 • edited Loading

nbys commented Aug 11, 2019

umputun commented Aug 11, 2019

nbys commented Aug 13, 2019 • edited Loading

umputun commented Aug 15, 2019

umputun commented Aug 15, 2019 • edited Loading

nbys commented Aug 15, 2019

umputun commented Feb 8, 2022

nbys commented Feb 9, 2022 • edited Loading

nbys left a comment •

edited

Loading

coveralls commented Jul 28, 2019 •

edited

Loading

nbys Jul 28, 2019 •

edited

Loading

nbys commented Aug 2, 2019 •

edited

Loading

nbys commented Aug 5, 2019 •

edited

Loading

umputun commented Aug 10, 2019 •

edited

Loading

nbys commented Aug 13, 2019 •

edited

Loading

umputun commented Aug 15, 2019 •

edited

Loading

nbys commented Feb 9, 2022 •

edited

Loading