Refactors the JWT token generator and adds unit tests for it. #1061
Conversation
alesstimec
requested review from
kian99,
ale8k,
mina1460 and
babakks
and removed request for
kian99
| zapctx.Error(ctx, "failed to fetch controller", zap.Error(err)) | ||
| return nil, errors.E(op, "failed to fetch controller", err) | ||
| } | ||
| for _, cloudRegion := range ctl.CloudRegions { |
There was a problem hiding this comment.
Are cloud regions what we want here instead of just clouds? There could be quite a lot of cloudRegions versus just Clouds right?
There was a problem hiding this comment.
well.. these are named cloud regions.. but what they are is cloud region priorities.. so each cloud region will have a priority set on a controller.. usually each controller won't have more than one priority entry per cloud region.. but you are correct.. i really should collect just clouds to reduce potential duplicate checks
internal/jimm/access_test.go
Outdated
| return nil | ||
| } | ||
|
|
||
| type testAccessChecker struct { |
There was a problem hiding this comment.
I like the structure of these mock checkers. Easy to use and extend.
There was a problem hiding this comment.
Small thing but what about putting them at the top of the file instead? Then you encounter them first before you use them.
There was a problem hiding this comment.
historically we've always put mock/test implementation at the end of file..but happy to change that practice
| // to cachedPerms if they exist. If the user does not have any of the desired permissions then an | ||
| // error is returned. | ||
| // Note that cachedPerms map is modified and returned. | ||
| func checkPermission(ctx context.Context, user *openfga.User, cachedPerms map[string]string, desiredPerms map[string]interface{}) (map[string]string, error) { | ||
| const op = errors.Op("jimm.checkPermission") | ||
| func (j *JIMM) CheckPermission(ctx context.Context, user *openfga.User, cachedPerms map[string]string, desiredPerms map[string]interface{}) (map[string]string, error) { |
There was a problem hiding this comment.
Would be good to ensure we have tests for this function too, but I can add that in a follow up if we don't
| @@ -131,7 +131,7 @@ func modelInfoFromPath(path string) (uuid string, finalPath string, err error) { | |||
|
|
|||
| // ServeWS implements jimmhttp.WSServer. | |||
| func (s modelProxyServer) ServeWS(ctx context.Context, clientConn *websocket.Conn) { | |||
| jwtGenerator := jimm.NewJwtGenerator(s.jimm) | |||
| jwtGenerator := jimm.NewJWTGenerator(s.jimm.Authenticator, &s.jimm.Database, s.jimm, s.jimm.JWTService) | |||
There was a problem hiding this comment.
It feels like we could create a nice separation of concerns here. Notice that we pass in s.jimm to satisfy the JWTGeneratorAccessChecker interface. This feels like the responsibility of an s.jimm.Authorizer object. Also better to go in a separate PR.
There was a problem hiding this comment.
hmm.. yes... let's do it in a separate PR, please..
alesstimec
force-pushed
the
refactor-make-token
branch
from
3f5d8b8 to
399a3a4
Compare
alesstimec
force-pushed
the
refactor-make-token
branch
from
f980141 to
1d4e128
Compare
Description
The what and why - include a summary of the change, describe what it does, and include relevant motivation and context.
Fixes JIRA/GitHub issue number
Engineering checklist
Check only items that apply
Test instructions
Notes for code reviewers