-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Feature/field restrictions #3041
Conversation
f53f9c1
to
c1d760c
Compare
ddfc0df
to
0a9ec86
Compare
dc54397
to
010c3ae
Compare
010c3ae
to
837c46f
Compare
gateway/api_definition.go
Outdated
@@ -17,6 +17,8 @@ import ( | |||
"text/template" | |||
"time" | |||
|
|||
"github.com/jensneuse/graphql-go-tools/pkg/graphql" |
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.
could we group imports?
like
import (
std_imports
third party
tyk
)
@@ -403,6 +403,20 @@ func (t BaseMiddleware) ApplyPolicies(session *user.SessionState) error { | |||
} | |||
} | |||
|
|||
for _, t := range v.RestrictedTypes { |
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.
do we support partitioning of restricted types?
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 know it is unrelated to this PR but could we make iteration variables names more meaningful?
like v -> accessRight
and k -> apiID
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.
You are right, it is very unclear but better to make it in another PR as it will change a lot of unrelated lines
gateway/mw_granular_access.go
Outdated
|
||
return nil, http.StatusOK | ||
|
||
} else { |
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.
else
branch is not required here as you already have returns in if
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.
we will have to cover ACL merging with tests
Fixes #3017