-
Notifications
You must be signed in to change notification settings - Fork 10
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
Local Authorization Check for RBAC #157
Conversation
nikhil-stytch
commented
Dec 6, 2023
•
edited
Loading
edited
- Works alongside sdk-codegen changes
- Validated that local authorization_check and new client constructions work as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few comments that should be addressed, but I'm going to approve to unblock. Can you change this to merge into v12-prerelease
?
stytch/b2b/rbac.go
Outdated
lastUpdatedAt time.Time | ||
} | ||
|
||
const refreshCadence = 300 * time.Second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meta-thought: I don't think the XClient
s 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely a MAJOR update, I'm going to cut a v12 branch to prepare a few things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's have this merge into v12-prerelease
branch
) | ||
assert.NoError(t, err) | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👨🍳 💋 Great tests
35b2d06
to
f71a617
Compare