Skip to content
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

feat: batch check relations #1521

Merged
merged 16 commits into from
Jul 18, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 120 additions & 0 deletions internal/check/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,15 @@ func NewHandler(d handlerDependencies) *Handler {
const (
RouteBase = "/relation-tuples/check"
OpenAPIRouteBase = RouteBase + "/openapi"
BatchRoute = "/relation-tuples/batchCheck"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about /relation-tuples/batch/check? Then we can have other batch ops in the future.

)

func (h *Handler) RegisterReadRoutes(r *x.ReadRouter) {
r.GET(RouteBase, h.getCheckMirrorStatus)
r.GET(OpenAPIRouteBase, h.getCheckNoStatus)
r.POST(RouteBase, h.postCheckMirrorStatus)
r.POST(OpenAPIRouteBase, h.postCheckNoStatus)
r.POST(BatchRoute, h.postBatchCheckMirrorStatus)
}

func (h *Handler) RegisterReadGRPC(s *grpc.Server) {
Expand Down Expand Up @@ -329,3 +331,121 @@ func (h *Handler) Check(ctx context.Context, req *rts.CheckRequest) (*rts.CheckR
Snaptoken: "not yet implemented",
}, nil
}

// Post Batch Check Permission Or Error Request Parameters
//
// swagger:parameters postBatchCheckPermissionOrError
//
//lint:ignore U1000 Used to generate Swagger and OpenAPI definitions
type postBatchCheckPermissionOrError struct {
// in: query
MaxDepth int `json:"max-depth"`

// in: body
Body postBatchCheckPermissionOrErrorBody
}

// Post Batch Check Permission Or Error Body
//
// swagger:model postBatchCheckPermissionOrErrorBody
//
//lint:ignore U1000 Used to generate Swagger and OpenAPI definitions
type postBatchCheckPermissionOrErrorBody struct {
Tuples []*ketoapi.RelationTuple `json:"tuples"`
}

// Batch Check Permission Result
//
// swagger:model batchCheckPermissionResult
type BatchCheckPermissionResult struct {
// An array of whether the relation tuple is allowed. The order aligns with the input order.
//
// required: true
Results []*CheckPermissionResult `json:"results"`
}

// swagger:route POST /relation-tuples/batchCheck permission postBatchCheckPermissionOrErrorBody
//
// # Batch check permissions
//
// To learn how relationship tuples and the check works, head over to [the documentation](https://www.ory.sh/docs/keto/concepts/api-overview).
//
// Consumes:
// - application/json
//
// Produces:
// - application/json
//
// Schemes: http, https
//
// Responses:
// 200: batchCheckPermissionResult
// 400: errorGeneric
// default: errorGeneric
func (h *Handler) postBatchCheckMirrorStatus(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
results, err := h.postBatchCheck(r.Context(), r.Body, r.URL.Query())
if err != nil {
h.d.Writer().WriteError(w, r, err)
return
}

h.d.Writer().Write(w, r, &BatchCheckPermissionResult{Results: results})
}

func (h *Handler) postBatchCheck(ctx context.Context, body io.Reader, query url.Values) ([]*CheckPermissionResult, error) {
maxDepth, err := x.GetMaxDepthFromQuery(query)
if err != nil {
return nil, err
}

var request postBatchCheckPermissionOrErrorBody
if err := json.NewDecoder(body).Decode(&request); err != nil {
return nil, errors.WithStack(herodot.ErrBadRequest.WithErrorf("could not unmarshal json: %s", err.Error()))
}

results := make([]*CheckPermissionResult, len(request.Tuples))
for i, tuple := range request.Tuples {
t, err := h.d.ReadOnlyMapper().FromTuple(ctx, tuple)
// herodot.ErrNotFound occurs when the namespace is unknown
if errors.Is(err, herodot.ErrNotFound) {
results[i] = &CheckPermissionResult{
Allowed: false,
}
continue
} else if err != nil {
return nil, err
}
allowed, err := h.d.PermissionEngine().CheckIsMember(ctx, t[0], maxDepth)
if err != nil {
return nil, err
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we annotate here which tuple caused the error? It might be frustrating to submit a batch of checks and get an error and then having to binary-search which tuple caused the error.

Alternatively we could just add the error to the results list and then commence with the next tuple. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding more context to the error is a good idea. The question then is whether to return errors at the tuple level vs for the API as a whole. If there are situations where clients might expect an error and would want to treat an error the same as receiving a false allowed value, I could see a benefit in continuing on error and returning for each tuple if an error was generated. As far as I know though, that isn't a normal case, so I will just add tuple details to the error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed my mind and went for proceeding with checking the whole batch and adding the error to the response for each tuple

}
results[i] = &CheckPermissionResult{
Allowed: allowed,
}
}

return results, nil
}

func (h *Handler) BatchCheck(ctx context.Context, req *rts.BatchCheckRequest) (*rts.BatchCheckResponse, error) {
responses := make([]*rts.CheckResponse, len(req.Tuples))
for i, reqTuple := range req.Tuples {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is duplicated from the REST handler here. Ideally we'd implement this once and have gRPC and REST call that function.

tuple := (&ketoapi.RelationTuple{}).FromProto(reqTuple)
internalTuple, err := h.d.ReadOnlyMapper().FromTuple(ctx, tuple)
if err != nil {
return nil, err
}
allowed, err := h.d.PermissionEngine().CheckIsMember(ctx, internalTuple[0], int(req.MaxDepth))
if err != nil {
return nil, err
}
responses[i] = &rts.CheckResponse{
Allowed: allowed,
Snaptoken: "not yet implemented",
}
}

return &rts.BatchCheckResponse{
Results: responses,
}, nil
}
4 changes: 4 additions & 0 deletions internal/httpclient/.openapi-generator/FILES

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions internal/httpclient/README.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

51 changes: 51 additions & 0 deletions internal/httpclient/api/openapi.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading