Skip to content

Conversation

jbielick
Copy link
Contributor

@jbielick jbielick commented Apr 2, 2020

Description

Logs the original request error for group membership check to google

Motivation and Context

When type asserting fails here, err is reassigned with nil and the default block of the switch prints out <nil> in the error message. This makes debugging a configuration or access token issue difficult.

Before:

[2020/04/02 17:28:05] [google.go:224] error checking group membership: <nil>

After:

[2020/04/02 17:28:05] [google.go:224] error checking group membership: Get "https://www.googleapis.com/admin/directory/v1/groups/group%40example.com/hasMember/josh%40example.com?alt=json&prettyPrint=false": oauth2: cannot fetch token: 401 Unauthorized
Response: {
  "error": "unauthorized_client",
  "error_description": "Client is unauthorized to retrieve access tokens using this method, or client not authorized for any of the scopes requested."
}

The issue seemed to be that I needed an additional OAuth scope, https://www.googleapis.com/auth/admin.directory.group.member.readonly, which I intend to add to the docs in another PR. The Admin SDK API was not enabled.

How Has This Been Tested?

I'm currently trying to get this working with nginx-ingress-controller. I cloned this repo, made the change, ran the tests, built the image, pushed it, and used my custom image and got the error message in my logs.

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

for _, group := range groups {
// 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()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

err here is what I want to print.

r, err := req.Do()
if err != nil {
err, ok := err.(*googleapi.Error)
gerr, ok := err.(*googleapi.Error)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reassignment of err here writes nil to err when the type assertion fails. I don't know what type err is before this. The error object is:

oauth2: cannot fetch token: 401 Unauthorized
Response: {
  "error": "unauthorized_client",
  "error_description": "Client is unauthorized to retrieve access tokens using this method, or client not authorized for any of the scopes requested."
}

@jbielick jbielick changed the title [provider/group] Always log request error object [provider/google] Always log hasMember request error object Apr 2, 2020
Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

This makes sense! Please add an entry to the changelog and we can get it merged, thanks for the fix!

@JoelSpeed JoelSpeed added the bug label Apr 4, 2020
when type asserting fails here, err is reassigned with nil and the
default block of the switch prints out <nil> in the error message. This
makes debugging a configuration or access token issue difficult

The particular error this surfaces is:

Response: {
  "error": "unauthorized_client",
  "error_description": "Client is unauthorized to retrieve access tokens using this method, or client not authorized for any of the scopes requested."
}

Signed-off-by: Josh Bielick <jbielick@gmail.com>
@jbielick
Copy link
Contributor Author

jbielick commented Apr 4, 2020

@JoelSpeed sure thing. Updated.

Copy link
Contributor

@loshz loshz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@loshz loshz merged commit f9f98cb into oauth2-proxy:master Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants