Add OAuth2 userinfo endpoint #14938

titpetric · 2021-03-10T00:37:36Z

This is an attempt to implement the userinfo endpoint in #8534

techknowlogick · 2021-03-10T00:43:59Z

routers/user/oauth_userinfo.go

+
+// UserInfoOAauth responds with OAuth formatted userinfo
+func UserInfoOAuth(ctx *context.Context) {
+	user := ctx.User


Thanks for this PR!

There is a chance that user may be nil, could you do a check that user != nil to ensure a nil referenceerror doesn't happen?

I think this may be more complicated. Not the nil check, but the correctness of the userinfo endpoint itself. AuthorizeOAuth reads the bearer token, and I suspect this code may need to do it too. I am relying on reqSignIn, but I didn't see any Authorization header parsing, so I suspect this may need a full rewrite.

Particularly, the reqSignIn requires a sign-in and should error out with a 403 before reaching this code.

Can you confirm that the required signed in context takes the Authorization header?

userinfo endpoint should work exclusively with token provided as this is endpoint that is called as s2s request so there won't be user cookies etc for user session

routers/user/oauth_userinfo.go

Co-authored-by: John Olheiser <[email protected]>

routers/user/oauth_userinfo.go

lunny · 2021-03-10T01:11:55Z

routers/routes/web.go

@@ -385,6 +385,7 @@ func RegisterRoutes(m *web.Route) {
 	m.Any("/user/events", reqSignIn, events.Events)

 	m.Group("/login/oauth", func() {
+		m.Get("/userinfo", user.UserInfoOAuth)


It must be confirmed to user login

Can you elaborate?

Should it be m.Get("/userinfo", reqSignIn, user.UserInfoOAuth)? Or should I read the Authorization HTTP header in the UserInfoOAuth, as it's done in AuthorizeOAuth?

We should also verify the access token here? See https://connect2id.com/products/server/docs/api/userinfo

reqSignIn will redirect to login page which is not suitable. An OAuth2 access token is required here.

@lunny you can access this URL with an OAuth2 token, and a 302 redirect to login page would be treated as invalid from the oauth client.

titpetric · 2021-03-11T08:31:26Z

I figured this might be the case. I'll sort it out :) thanks for confirming

…

On Wed, Mar 10, 2021, 22:40 Lauris BH ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In routers/user/oauth_userinfo.go <#14938 (comment)>: > + + "code.gitea.io/gitea/modules/context" +) + +// UserInfoResponse represents a successful userinfo response +type UserInfoResponse struct { + Sub string `json:"sub"` + Name string `json:"name"` + Username string `json:"preffered_username"` + Email string `json:"email"` + Picture string `json:"picture"` +} + +// UserInfoOAauth responds with OAuth formatted userinfo +func UserInfoOAuth(ctx *context.Context) { + user := ctx.User userinfo endpoint should work exclusively with token provided as this is endpoint that is called as s2s request so there won't be user cookies etc for user session — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#14938 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABY7EH3UBV62AUCF2TX42DTC7RNPANCNFSM4Y4ZSCAQ> .

techknowlogick · 2021-04-08T23:03:24Z

I've just resolved the conflicts with this PR

routers/user/oauth_userinfo.go

routers/routes/web.go

routers/user/oauth_userinfo.go

Co-authored-by: 6543 <[email protected]>

lunny

See #14938 (comment)

techknowlogick · 2021-04-11T16:37:39Z

routers/user/oauth_userinfo.go

+	user := ctx.User
+	response := &userInfoResponse{
+		Sub:      fmt.Sprint(user.ID),
+		Name:     user.FullName,


Sometimes fullname is empty, this could have a fallback if fullname is empty then use Name

technically per spec, this isn't required, but if it is empty they recommend not including it. So I won't block on this.

mcansky · 2021-04-12T16:41:43Z

that's looking very promising 🍻

mcansky · 2021-04-22T14:41:58Z

just a little nudge : anything we can do to help ? is it testable yet to provide some feedback ?

6543 · 2021-04-22T14:45:27Z

@mcansky it is testable - the only nit is, it will redirect if user is not authenticated - witch is not the expected behaviour as by specs if I understand it correctly

If you can test & give us feedback, this would be awesome :)

codecov-commenter · 2021-04-22T15:18:14Z

Codecov Report

Merging #14938 (3ba7c55) into master (72e0ad8) will decrease coverage by 0.02%.
The diff coverage is 10.00%.

@@            Coverage Diff             @@
##           master   #14938      +/-   ##
==========================================
- Coverage   43.92%   43.90%   -0.03%     
==========================================
  Files         678      679       +1     
  Lines       81739    81749      +10     
==========================================
- Hits        35902    35890      -12     
- Misses      39982    39999      +17     
- Partials     5855     5860       +5

Impacted Files	Coverage Δ
routers/user/oauth_userinfo.go	`0.00% <0.00%> (ø)`
routers/routes/web.go	`86.45% <100.00%> (+0.01%)`	⬆️
modules/util/timer.go	`42.85% <0.00%> (-42.86%)`	⬇️
modules/queue/manager.go	`61.36% <0.00%> (-2.85%)`	⬇️
routers/api/v1/repo/pull.go	`28.14% <0.00%> (-0.60%)`	⬇️
services/pull/pull.go	`43.47% <0.00%> (-0.46%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72e0ad8...3ba7c55. Read the comment docs.

lunny · 2021-04-22T15:41:18Z

@mcansky it is testable - the only nit is, it will redirect if user is not authenticated - witch is not the expected behaviour as by specs if I understand it correctly

If you can test & give us feedback, this would be awesome :)

I don't think so. The main problem is the endpoint has no token protected.

techknowlogick · 2021-05-06T05:03:30Z

Closing per #15721

Add OAuth2 userinfo endpoint

adc441f

techknowlogick reviewed Mar 10, 2021

View reviewed changes

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 10, 2021

techknowlogick reviewed Mar 10, 2021

View reviewed changes

routers/user/oauth_userinfo.go Outdated Show resolved Hide resolved

jolheiser requested changes Mar 10, 2021

View reviewed changes

routers/user/oauth_userinfo.go Outdated Show resolved Hide resolved

Update routers/user/oauth_userinfo.go

e7c7fc3

Co-authored-by: John Olheiser <[email protected]>

lunny reviewed Mar 10, 2021

View reviewed changes

routers/user/oauth_userinfo.go Show resolved Hide resolved

lunny reviewed Mar 10, 2021

View reviewed changes

routers/user/oauth_userinfo.go Outdated Show resolved Hide resolved

lunny reviewed Mar 10, 2021

View reviewed changes

Tit Petric added 2 commits March 10, 2021 02:19

Add copyright header to userinfo endpoint

d64c427

Fix: OAuth userinfo endpoint now returns absolute URL

ac982a8

artizirk mentioned this pull request Mar 25, 2021

Implement well known OpenID Configuration #8775

Closed

lunny added this to the 1.15.0 milestone Mar 25, 2021

lunny added the type/enhancement An improvement of existing functionality label Mar 25, 2021

6543 and others added 2 commits March 30, 2021 19:34

Merge branch 'master' into master

221408f

Merge branch 'master' into master

91619e7

6543 reviewed Apr 9, 2021

View reviewed changes

routers/user/oauth_userinfo.go Show resolved Hide resolved

6543 reviewed Apr 9, 2021

View reviewed changes

routers/user/oauth_userinfo.go Outdated Show resolved Hide resolved

routers/user/oauth_userinfo.go Outdated Show resolved Hide resolved

6543 added 2 commits April 9, 2021 05:32

make linter happy

4d87466

Merge branch 'master' into master

f36725e

6543 reviewed Apr 9, 2021

View reviewed changes

routers/routes/web.go Outdated Show resolved Hide resolved

6543 reviewed Apr 9, 2021

View reviewed changes

routers/user/oauth_userinfo.go Outdated Show resolved Hide resolved

keep linter quiet

ebe2d37

6543 reviewed Apr 9, 2021

View reviewed changes

routers/user/oauth_userinfo.go Outdated Show resolved Hide resolved

routers/user/oauth_userinfo.go Outdated Show resolved Hide resolved

techknowlogick and others added 4 commits April 9, 2021 16:57

Update routers/user/oauth_userinfo.go

7a2ca76

Co-authored-by: 6543 <[email protected]>

Update routers/user/oauth_userinfo.go

4209065

Co-authored-by: 6543 <[email protected]>

Merge branch 'master' into master

dc29ed2

Update oauth_userinfo.go

1489f99

techknowlogick approved these changes Apr 9, 2021

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 9, 2021

lunny requested changes Apr 10, 2021

View reviewed changes

techknowlogick reviewed Apr 11, 2021

View reviewed changes

6543 mentioned this pull request Apr 14, 2021

add well-known config for OIDC #15355

Merged

techknowlogick added 2 commits April 15, 2021 22:35

Merge branch 'master' into master

36e6818

Update oidc_wellknown.tmpl

0a3aea7

Merge branch 'master' into master

3ba7c55

NLH-Software mentioned this pull request May 4, 2021

Feature/oauth userinfo #15721

Merged

techknowlogick closed this May 6, 2021

techknowlogick removed this from the 1.15.0 milestone May 6, 2021

go-gitea locked and limited conversation to collaborators Oct 19, 2021

Uh oh!

Add OAuth2 userinfo endpoint #14938

Add OAuth2 userinfo endpoint #14938

Uh oh!

Conversation

titpetric commented Mar 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

titpetric Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

titpetric commented Mar 11, 2021 via email

Uh oh!

techknowlogick commented Apr 8, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lunny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcansky commented Apr 12, 2021

Uh oh!

mcansky commented Apr 22, 2021

Uh oh!

6543 commented Apr 22, 2021

Uh oh!

codecov-commenter commented Apr 22, 2021

Codecov Report

Uh oh!

lunny commented Apr 22, 2021

Uh oh!

techknowlogick commented May 6, 2021

Uh oh!

Uh oh!

titpetric Mar 10, 2021 •

edited

Loading