Implement Token Management - Registration and Logout

[sbryzak]

This pull request implements token management functionality.

• Sets LOGGED_OUT status for all user tokens when user logs out
• Registers access and refresh tokens when user logs in
• Registers new access and refresh tokens when token is refreshed

Fixes #726

[alien-ike]

Ike Plugins (test-keeper)

Thank you @sbryzak for this contribution!

It seems that this PR already contains some added or changed tests. Good job!

Your plugin configuration is stored in the file.

[codecov[bot]]

Codecov Report

Merging #746 into master will decrease coverage by 0.18%. The diff coverage is 74.22%.

@@            Coverage Diff             @@
##           master     #746      +/-   ##
==========================================
- Coverage   79.03%   78.85%   -0.19%     
==========================================
  Files          94       94              
  Lines        8639     8660      +21     
==========================================
+ Hits         6828     6829       +1     
- Misses       1331     1347      +16     
- Partials      480      484       +4

Impacted Files	Coverage Δ
...rovider/service/authentication_provider_service.go	74.03% <ø> (+1.86%)	:arrow_up:
authorization/token/manager/token_manager.go	82.16% <100%> (+0.23%)	:arrow_up:
authentication/logout/service/logout_service.go	55.31% <60%> (+0.55%)	:arrow_up:
authorization/token/service/token_service.go	72.97% <71.97%> (-3.89%)	:arrow_down:
controller/logout.go	91.3% <76.47%> (-8.7%)	:arrow_down:
controller/token.go	84.54% <80%> (ø)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 87d9456...6cb6662. Read the comment docs.

[sbryzak]

@alexeykazakov thanks, I'm just working on a last minute issue that has popped up... when we manually populate the token claims for testing (in this method) the .Raw() function doesn't return the token string, which means when I call it in logout_service.go it currently returns an empty string and so the test fails. I'm currently trying to find a workaround for this.

[sbryzak]

I've resolved the issue with the token Raw() function, however another issue has presented itself.. how should we handle requests where the token contains a sub claim that doesn't represent an identity that exists in the database? For testing we are using testsupport.TestIdentity which isn't backed up by an actual identity record in the database, which is causing the endpoint to fail. @alexeykazakov @xcoulon How do you guys think we should handle this?

[xcoulon]

I've resolved the issue with the token Raw() function, however another issue has presented itself.. how should we handle requests where the token contains a sub claim that doesn't represent an identity that exists in the database? For testing we are using testsupport.TestIdentity which isn't backed up by an actual identity record in the database, which is causing the endpoint to fail. @alexeykazakov @xcoulon How do you guys think we should handle this?

@sbryzak you mean, a token was issues (the user existed in the DB), but before the user tries to logout, the identity/user records were removed from the db and hence you can't look them up during the logout? That would be a very rare edge case, so I suggest we log an error and forward the user to an error page (if that's possible?) Does it make sense?

[sbryzak]

@xcoulon I guess I'm more questioning the nature of the tests, take this line of code from logout_blackbox_test.go for example:

svc, ctrl := s.UnSecuredController()

What does the "unsecured controller" function actually mean? It's still calling testsupport.ServiceAsUser() which under the covers contains this code:

    svc := goa.New(serviceName)
    svc.Context = WithIdentity(svc.Context, u)
    svc.Context = manager.ContextWithTokenManager(svc.Context, testtoken.TokenManager)
    return svc

So we're creating a new service instance, with a context that contains an access token - so why is this an "unsecured" controller? @alexeykazakov perhaps you could shed some light on this. Also, the existing logout endpoint isn't secured (i.e. there's no jwt security constraint on the endpoint definition) so it's a little confusing as to why the tests for this endpoint are using a context that contains an access token.

[alexeykazakov]

If our code expects a token for an existing user than let’s just create that user in DB and then use its token in the tests. Unsecured controllers are used for testing endpoints without providing tokens. If we embed a token into the context in unsecured controller then it’s a bug in the test. We ether should rename it to secured controller or remove the token from it. If you are talking about the old logout endpoint then yes we should not embed a token since our endpoint doesn’t require it.

[sbryzak]

@alexeykazakov I've fixed the test suite so that the unsecured controller is now actually unsecured (i.e. no token). Everything passes now, so would be great if you and/or @xcoulon could take one last look at the PR to give me the ok to merge.

[sbryzak]

@alexeykazakov I've modified the doLogout() function to return json errors instead.

[alexeykazakov]

Use errors.NewBadParameter() instead of explicit goa.BadRequest as we do in other places.

Besides that looks good.

[sbryzak]

@alexeykazakov goa.BadRequest was the original code, changing it to use errors.NewBadParameter() instead now causes test failures... it seems that the ErrorToJSONAPIError() function is returning an internal server errors for a BadParameterError error type, which is really weird. I'm currently trying to work out why this is happening.

[sbryzak]

@alexeykazakov nevermind... it had imported the fabric8-common errors package, therefore the error types didn't match. Fixed now.