Skip to content

Add OAuth2 userinfo endpoint #14938

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.

Sign up for GitHub

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

Closed
wants to merge 16 commits into from
Closed

Add OAuth2 userinfo endpoint #14938

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
adc441f
Add OAuth2 userinfo endpoint
Mar 10, 2021
e7c7fc3
Update routers/user/oauth_userinfo.go
titpetric Mar 10, 2021
d64c427
Add copyright header to userinfo endpoint
Mar 10, 2021
ac982a8
Fix: OAuth userinfo endpoint now returns absolute URL
Mar 10, 2021
221408f
Merge branch 'master' into master
6543 Mar 30, 2021
91619e7
Merge branch 'master' into master
techknowlogick Apr 8, 2021
4d87466
make linter happy
6543 Apr 9, 2021
f36725e
Merge branch 'master' into master
6543 Apr 9, 2021
ebe2d37
keep linter quiet
6543 Apr 9, 2021
7a2ca76
Update routers/user/oauth_userinfo.go
techknowlogick Apr 9, 2021
4209065
Update routers/user/oauth_userinfo.go
techknowlogick Apr 9, 2021
dc29ed2
Merge branch 'master' into master
techknowlogick Apr 9, 2021
1489f99
Update oauth_userinfo.go
techknowlogick Apr 9, 2021
36e6818
Merge branch 'master' into master
techknowlogick Apr 16, 2021
0a3aea7
Update oidc_wellknown.tmpl
techknowlogick Apr 16, 2021
3ba7c55
Merge branch 'master' into master
6543 Apr 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions routers/routes/web.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It must be confirmed to user login

Copy link
Author

@titpetric titpetric Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

m.Get("/authorize", bindIgnErr(auth.AuthorizationForm{}), user.AuthorizeOAuth)
m.Post("/grant", bindIgnErr(auth.GrantApplicationForm{}), user.GrantApplicationOAuth)
// TODO manage redirection
Expand Down
29 changes: 29 additions & 0 deletions routers/user/oauth_userinfo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package user

import (
"fmt"

"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:"preferred_username"`
Email string `json:"email"`
Picture string `json:"picture"`
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

response := &UserInfoResponse{
Sub: fmt.Sprint(user.ID),
Name: user.FullName,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Username: user.Name,
Email: user.Email,
Picture: user.Avatar,
}
ctx.JSON(200, response)
}