Skip to content
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.

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

Added API error response handling #13

Merged
merged 2 commits into from
Oct 27, 2019
Merged

Conversation

ppacher
Copy link
Contributor

@ppacher ppacher commented Oct 10, 2019

This PR adds a custom error type that is populated with the field errors returned by the API.
With this changes it's easier to debug why the authn-server replied with 4xx or 5xx status codes and allows to inspect the error returned:

id, err := cli.ImportAccount(username, password, locked);
if respErr, ok := err.(*authn.ErrorResponse); ok {
    if userNameErr, ok := respErr.Field("username"); ok {
         fmt.Println("username is " + userNameErr)
    }
}

I've not yet updated the test-cases because it would require returning a JSON object for errors and not just a statusCode and wanted to ask if you would even consider merging this PR.

Thanks

@cainlevy
Copy link
Member

This is certainly a welcome addition

@ppacher
Copy link
Contributor Author

ppacher commented Oct 18, 2019

@cainlevy not directly related to this PR but while using authn-go in a SSO environment I had a problem with audience verification and "fixed" that in the last commit here. I can remove that change and create a new PR if you like. Here's the problem I had:

I'm using authn-server for the authentication part at, let's say, auth.example.com. I'm also having a UI under that domain the allows the user to login, change it's password, .... .

In addition, I'm having an identity and access management server at iam.example.com that stores the user profile and is responsible for access and permission handling. Now I have multiple applications that use an SSO approach by importSession and getting a fresh access token for the domain they are running on (frontend + backend micro service). I.e. app.example.com uses the refresh token stored in the cookie to get an access token for the app.example.com audience. So far everything works but now all of those applications need to load the user-profile from iam.example.com. It now rejects the access token because it's for app.example.com and not for iam.example.com. For this problem there are IMHO two solutions:

  • automatically add an additional audience to the JWT (in that case iam.example.com)
  • not verifying the audience and accepting the JWT as long as it's published by authn-server.

Unfortunately I didn't find a way to tell authn-server to add an additional audience to every token (if there is, please point me to the docs) so I went with the second one. Here's the problem, authn-go always sets jwt.Audience{verifier.audience} even if verify.audience is empty. That requires that the JWT has a "" audience instead of not verifying the audience. The change in the last commit does exactly that, if no audience is configured for the verifier/client, the audience will not be verified. Though, I'm not sure if this should count as a backwards-incompatible change and if people are relying on that behavior. In this case we might think about adding it to the new client when doing #12.

@cainlevy
Copy link
Member

cainlevy commented Oct 20, 2019

@ppacher would it be appropriate to configure your iam.example.com service so that it accepts tokens issued to other audiences? it would mean you have a centralized place that must know every possible SSO consumer.

(if there's more to discuss here we should open a separate issue to track the conversation)

@ppacher
Copy link
Contributor Author

ppacher commented Oct 20, 2019

@cainlevy configuring iam.example.com to accept tokens from/for different audiences would be the best way as I can use a whitelist approach. Though this also seems to be unsupported by authn-go right now because when passing a []string to verifier.audience it fails if not all of those audiences are part of the JWT. So instead of checking if one is there it enforces all of them. That's why I went with disabling verification for now. Still, both solutions would require a change to authn-go.

Although I'd prefer the whitelist approach I also think that support to disable audience verification would be a helpful addition to authn-go

@ppacher
Copy link
Contributor Author

ppacher commented Oct 21, 2019

I created a dedicated issue for this discussion and will revert the last commit for now

Copy link
Member

@cainlevy cainlevy left a comment

Choose a reason for hiding this comment

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

I think this changeset will be ready after test cases and cleanup 👍

@ppacher
Copy link
Contributor Author

ppacher commented Oct 24, 2019

I'll try to get the test cases running on the weekend. Would you expect that all test-cases in internal_client_test.go use and verify the new ErrorResponse (because that's basically copy-pasting) or would it be enough to test the error handling of internalClient.doWithAuth?

@cainlevy
Copy link
Member

My preference is to test the public API, but I'm content to pick any function that might error as a canary for the expected behavior.

@ppacher
Copy link
Contributor Author

ppacher commented Oct 26, 2019

Added the test cases. If you like something changed just tell me

@ppacher ppacher requested a review from cainlevy October 26, 2019 08:23
Copy link
Member

@cainlevy cainlevy left a comment

Choose a reason for hiding this comment

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

looks good, thanks!

@cainlevy cainlevy merged commit aa71037 into keratin:master Oct 27, 2019
@ppacher ppacher deleted the api-errors branch October 27, 2019 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants