Local Authorization Check for RBAC #157
Conversation
nikhil-stytch
commented
Dec 6, 2023
•
nikhil-stytch
commented
- Works alongside sdk-codegen changes
- Validated that local authorization_check and new client constructions work as expected
nikhil-stytch
requested review from
jbolduc-stytch
and removed request for
jbolduc-stytch
nikhil-stytch
changed the title
stytch/b2b/rbac.go
Outdated
| lastUpdatedAt time.Time | ||
| } | ||
|
|
||
| const refreshCadence = 300 * time.Second |
There was a problem hiding this comment.
[dust] Would 5 * time.Minute be better here?
stytch/b2b/rbac.go
Outdated
| return time.Since(pc.lastUpdatedAt) > refreshCadence | ||
| } | ||
|
|
||
| func (pc *PolicyCache) Get(ctx context.Context) (*rbac.Policy, error) { | ||
| if pc.policy == nil || pc.shouldRefreshPolicy() { |
There was a problem hiding this comment.
Let's go all the way here
| return time.Since(pc.lastUpdatedAt) > refreshCadence | |
| } | |
| func (pc *PolicyCache) Get(ctx context.Context) (*rbac.Policy, error) { | |
| if pc.policy == nil || pc.shouldRefreshPolicy() { | |
| return pc.policy == nil || time.Since(pc.lastUpdatedAt) > refreshCadence | |
| } | |
| func (pc *PolicyCache) Get(ctx context.Context) (*rbac.Policy, error) { | |
| if pc.shouldRefreshPolicy() { |
| "github.com/stytchauth/stytch-go/v11/stytch/stytcherror" | ||
| ) | ||
|
|
||
| type SessionsClient struct { | ||
| C stytch.Client | ||
| JWKS *keyfunc.JWKS |
There was a problem hiding this comment.
Meta-thought: I don't think the XClients should be exposing any of these fields externally -- we should hide this and only expose the method calls and sub-services. With that said, that can go into the backlog for SDK improvements.
stytch/b2b/sessions.go
Outdated
| } | ||
|
|
||
| err = shared.PerformAuthorizationCheck(policy, claims.Roles, memberSession.OrganizationID, authorizationCheck) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
[boulder] This check belongs inside the if authorizationCheck != nil block -- otherwise you're about to do an auth check against a nil policy
There was a problem hiding this comment.
Ahh, I read below that shared.PerformAuthorizationCheck will check if the authCheck is nil, but I still think it'd be safer to put this inside the block above to be crystal clear.
| @@ -1,3 +1,3 @@ | |||
| package config | |||
|
|
|||
| const APIVersion = "11.5.2" | |||
| const APIVersion = "11.6.0" | |||
There was a problem hiding this comment.
Definitely a MAJOR update, I'm going to cut a v12 branch to prepare a few things
There was a problem hiding this comment.
Okay, let's have this merge into v12-prerelease branch
| ) | ||
| assert.NoError(t, err) | ||
| }) | ||
| } |
logan-stytch
force-pushed
the
nd/authz-2
branch
from
35b2d06
to
f71a617
Compare
