keratin / authn-go

Go client library for Keratin AuthN
https://github.com/keratin/authn-server
GNU Lesser General Public License v3.0
32 stars 10 forks source link

Added API error response handling #13

Closed ppacher closed 5 years ago

ppacher commented 5 years ago

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 commented 5 years ago

This is certainly a welcome addition

ppacher commented 5 years ago

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

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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

ppacher commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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