Check Google group membership with hasMember and get. (#224)
* Check Google group membership with hasMember and get. This PR is an enhancement built on https://github.com/pusher/oauth2_proxy/pull/160. That PR reduces the number of calls to the Google Admin API and simplifies the code by using the hasMember method. It also supports checking membership in nested groups. However, the above message doesn't handle members who are not a part of the domain. The hasMember API returns a 400 for that case. As a fallback, when the API returns a 400, this change will try using the `get` API which works as expected for members who aren't a part of the domain. Supporting members who belong to the Google group but aren't part of the domain is a requested feature from https://github.com/pusher/oauth2_proxy/issues/95. https://developers.google.com/admin-sdk/directory/v1/reference/members/get Note that nested members who are not a part of the domain will not be correctly detected with this change. * Update CHANGELOG. * Fix incorrect JSON and stop escaping strings. * Add comments for each scenario.
This commit is contained in:
Justin Palpant
Dan Bond
parent
69c723af81
commit
7d910c0ae8
@ -31,8 +31,9 @@
|
||||
|
||||
## Changes since v3.2.0
|
||||
|
||||
- [#178](https://github.com/pusher/oauth2_proxy/pull/178) Add Silence Ping Logging and Exclude Logging Paths flags (@kskewes)
|
||||
- [#209](https://github.com/pusher/oauth2_proxy/pull/209) Improve docker build caching of layers (@dekimsey)
|
||||
- [#224](https://github.com/pusher/oauth2_proxy/pull/224) Check Google group membership using hasMember to support nested groups and external users (@jpalpant)
|
||||
- [#178](https://github.com/pusher/outh2_proxy/pull/178) Add Silence Ping Logging and Exclude Logging Paths flags (@kskewes)
|
||||
- [#209](https://github.com/pusher/outh2_proxy/pull/209) Improve docker build caching of layers (@dekimsey)
|
||||
- [#186](https://github.com/pusher/oauth2_proxy/pull/186) Make config consistent (@JoelSpeed)
|
||||
- [#187](https://github.com/pusher/oauth2_proxy/pull/187) Move root packages to pkg folder (@JoelSpeed)
|
||||
- [#65](https://github.com/pusher/oauth2_proxy/pull/65) Improvements to authenticate requests with a JWT bearer token in the `Authorization` header via
|
||||
|
||||
@ -189,73 +189,44 @@ func getAdminService(adminEmail string, credentialsReader io.Reader) *admin.Serv
|
||||
}
|
||||
|
||||
func userInGroup(service *admin.Service, groups []string, email string) bool {
|
||||
user, err := fetchUser(service, email)
|
||||
if err != nil {
|
||||
logger.Printf("Warning: unable to fetch user: %v", err)
|
||||
user = nil
|
||||
}
|
||||
|
||||
for _, group := range groups {
|
||||
members, err := fetchGroupMembers(service, group)
|
||||
// Use the HasMember API to checking for the user's presence in each group or nested subgroups
|
||||
req := service.Members.HasMember(group, email)
|
||||
r, err := req.Do()
|
||||
if err != nil {
|
||||
if err, ok := err.(*googleapi.Error); ok && err.Code == 404 {
|
||||
logger.Printf("error fetching members for group %s: group does not exist", group)
|
||||
} else {
|
||||
logger.Printf("error fetching group members: %v", err)
|
||||
return false
|
||||
}
|
||||
}
|
||||
err, ok := err.(*googleapi.Error)
|
||||
if ok && err.Code == 404 {
|
||||
logger.Printf("error checking membership in group %s: group does not exist", group)
|
||||
} else if ok && err.Code == 400 {
|
||||
// It is possible for Members.HasMember to return false even if the email is a group member.
|
||||
// One case that can cause this is if the user email is from a different domain than the group,
|
||||
// e.g. "member@otherdomain.com" in the group "group@mydomain.com" will result in a 400 error
|
||||
// from the HasMember API. In that case, attempt to query the member object directly from the group.
|
||||
req := service.Members.Get(group, email)
|
||||
r, err := req.Do()
|
||||
|
||||
for _, member := range members {
|
||||
if member.Email == email {
|
||||
return true
|
||||
}
|
||||
if user == nil {
|
||||
continue
|
||||
}
|
||||
switch member.Type {
|
||||
case "CUSTOMER":
|
||||
if member.Id == user.CustomerId {
|
||||
return true
|
||||
}
|
||||
case "USER":
|
||||
if member.Id == user.Id {
|
||||
if err != nil {
|
||||
logger.Printf("error using get API to check member %s of google group %s: user not in the group", email, group)
|
||||
continue
|
||||
}
|
||||
|
||||
// If the non-domain user is found within the group, still verify that they are "ACTIVE".
|
||||
// Do not count the user as belonging to a group if they have another status ("ARCHIVED", "SUSPENDED", or "UNKNOWN").
|
||||
if r.Status == "ACTIVE" {
|
||||
return true
|
||||
}
|
||||
} else {
|
||||
logger.Printf("error checking group membership: %v", err)
|
||||
}
|
||||
continue
|
||||
}
|
||||
if r.IsMember {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
func fetchUser(service *admin.Service, email string) (*admin.User, error) {
|
||||
user, err := service.Users.Get(email).Do()
|
||||
return user, err
|
||||
}
|
||||
|
||||
func fetchGroupMembers(service *admin.Service, group string) ([]*admin.Member, error) {
|
||||
members := []*admin.Member{}
|
||||
pageToken := ""
|
||||
for {
|
||||
req := service.Members.List(group)
|
||||
if pageToken != "" {
|
||||
req.PageToken(pageToken)
|
||||
}
|
||||
r, err := req.Do()
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
for _, member := range r.Members {
|
||||
members = append(members, member)
|
||||
}
|
||||
if r.NextPageToken == "" {
|
||||
break
|
||||
}
|
||||
pageToken = r.NextPageToken
|
||||
}
|
||||
return members, nil
|
||||
}
|
||||
|
||||
// ValidateGroup validates that the provided email exists in the configured Google
|
||||
// group(s).
|
||||
func (p *GoogleProvider) ValidateGroup(email string) bool {
|
||||
|
||||
@ -1,6 +1,7 @@
|
||||
package providers
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/base64"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
@ -12,6 +13,7 @@ import (
|
||||
"github.com/stretchr/testify/assert"
|
||||
|
||||
admin "google.golang.org/api/admin/directory/v1"
|
||||
option "google.golang.org/api/option"
|
||||
)
|
||||
|
||||
func newRedeemServer(body []byte) (*url.URL, *httptest.Server) {
|
||||
@ -185,34 +187,53 @@ func TestGoogleProviderGetEmailAddressEmailMissing(t *testing.T) {
|
||||
|
||||
func TestGoogleProviderUserInGroup(t *testing.T) {
|
||||
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if r.URL.Path == "/users/member-by-email@example.com" {
|
||||
fmt.Fprintln(w, "{}")
|
||||
} else if r.URL.Path == "/users/non-member-by-email@example.com" {
|
||||
fmt.Fprintln(w, "{}")
|
||||
} else if r.URL.Path == "/users/member-by-id@example.com" {
|
||||
fmt.Fprintln(w, "{\"id\": \"member-id\"}")
|
||||
} else if r.URL.Path == "/users/non-member-by-id@example.com" {
|
||||
fmt.Fprintln(w, "{\"id\": \"non-member-id\"}")
|
||||
} else if r.URL.Path == "/groups/group@example.com/members" {
|
||||
fmt.Fprintln(w, "{\"members\": [{\"email\": \"member-by-email@example.com\"}, {\"id\": \"member-id\", \"type\": \"USER\"}]}")
|
||||
if r.URL.Path == "/groups/group@example.com/hasMember/member-in-domain@example.com" {
|
||||
fmt.Fprintln(w, `{"isMember": true}`)
|
||||
} else if r.URL.Path == "/groups/group@example.com/hasMember/non-member-in-domain@example.com" {
|
||||
fmt.Fprintln(w, `{"isMember": false}`)
|
||||
} else if r.URL.Path == "/groups/group@example.com/hasMember/member-out-of-domain@otherexample.com" {
|
||||
http.Error(
|
||||
w,
|
||||
`{"error": {"errors": [{"domain": "global","reason": "invalid","message": "Invalid Input: memberKey"}],"code": 400,"message": "Invalid Input: memberKey"}}`,
|
||||
http.StatusBadRequest,
|
||||
)
|
||||
} else if r.URL.Path == "/groups/group@example.com/hasMember/non-member-out-of-domain@otherexample.com" {
|
||||
http.Error(
|
||||
w,
|
||||
`{"error": {"errors": [{"domain": "global","reason": "invalid","message": "Invalid Input: memberKey"}],"code": 400,"message": "Invalid Input: memberKey"}}`,
|
||||
http.StatusBadRequest,
|
||||
)
|
||||
} else if r.URL.Path == "/groups/group@example.com/members/non-member-out-of-domain@otherexample.com" {
|
||||
// note that the client currently doesn't care what this response text or code is - any error here results in failure to match the group
|
||||
http.Error(
|
||||
w,
|
||||
`{"error": {"errors": [{"domain": "global","reason": "notFound","message": "Resource Not Found: memberKey"}],"code": 404,"message": "Resource Not Found: memberKey"}}`,
|
||||
http.StatusNotFound,
|
||||
)
|
||||
} else if r.URL.Path == "/groups/group@example.com/members/member-out-of-domain@otherexample.com" {
|
||||
fmt.Fprintln(w,
|
||||
`{"kind": "admin#directory#member","etag":"12345","id":"1234567890","email": "member-out-of-domain@otherexample.com","role": "MEMBER","type": "USER","status": "ACTIVE","delivery_settings": "ALL_MAIL"}}`,
|
||||
)
|
||||
}
|
||||
}))
|
||||
defer ts.Close()
|
||||
|
||||
client := ts.Client()
|
||||
service, err := admin.New(client)
|
||||
ctx := context.Background()
|
||||
|
||||
service, err := admin.NewService(ctx, option.WithHTTPClient(client))
|
||||
service.BasePath = ts.URL
|
||||
assert.Equal(t, nil, err)
|
||||
|
||||
result := userInGroup(service, []string{"group@example.com"}, "member-by-email@example.com")
|
||||
result := userInGroup(service, []string{"group@example.com"}, "member-in-domain@example.com")
|
||||
assert.True(t, result)
|
||||
|
||||
result = userInGroup(service, []string{"group@example.com"}, "member-by-id@example.com")
|
||||
result = userInGroup(service, []string{"group@example.com"}, "member-out-of-domain@otherexample.com")
|
||||
assert.True(t, result)
|
||||
|
||||
result = userInGroup(service, []string{"group@example.com"}, "non-member-by-id@example.com")
|
||||
result = userInGroup(service, []string{"group@example.com"}, "non-member-in-domain@example.com")
|
||||
assert.False(t, result)
|
||||
|
||||
result = userInGroup(service, []string{"group@example.com"}, "non-member-by-email@example.com")
|
||||
result = userInGroup(service, []string{"group@example.com"}, "non-member-out-of-domain@otherexample.com")
|
||||
assert.False(t, result)
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user