Fix spicedb throwing on invalid arguments #20269
Merged
+37
−24
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
geropl
reviewed
Oct 7, 2024
|
|
||
| // this pattern matches v4 UUIDs as well as the new generated workspace ids (e.g. pink-panda-ns35kd21) | ||
| const workspaceIDRegex = RegExp(`^(?:debug-)?${baseWorkspaceIDRegex}$`); | ||
| export const workspaceIDRegex = RegExp(`^(?:debug-)?${baseWorkspaceIDRegex}$`); |
There was a problem hiding this comment.
🫧 I wonder where this regex comes from... it hopefully is identical with this one?
@filiptronicek Could you check whether we can easily unify something here? 🤔 The code I cited should be the source of truth.
geropl
reviewed
Oct 7, 2024
| import { ContextParser } from "../workspace/context-parser-service"; | ||
| import { workspaceIDRegex } from "@gitpod/gitpod-protocol/lib/util/gitpod-host-url"; | ||
|
|
||
| const isWorkspaceId = (workspaceId?: string) => { |
There was a problem hiding this comment.
This kind of method should ideally be part of parse-workspace-id.ts.
geropl
reviewed
Oct 7, 2024
| async updateWorkspacePort(req: UpdateWorkspacePortRequest): Promise<UpdateWorkspacePortResponse> { | ||
| if (!req.workspaceId) { | ||
| throw new ApplicationError(ErrorCodes.BAD_REQUEST, "workspaceId is required"); | ||
| if (!isWorkspaceId(req.workspaceId)) { |
geropl
reviewed
Oct 7, 2024
| const permitted = response.permissionship === v1.CheckPermissionResponse_Permissionship.HAS_PERMISSION; | ||
| return { permitted, checkedAt: response.checkedAt?.token }; | ||
| } catch (err) { | ||
| if (isGrpcError(err) && err.code === grpc.status.INVALID_ARGUMENT) { |
There was a problem hiding this comment.
Please add a comment on the "why"/context 🙏
geropl
approved these changes
Oct 7, 2024
filiptronicek
added a commit
that referenced
this pull request
Oct 7, 2024
roboquat
pushed a commit
that referenced
this pull request
Oct 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
If you called
WorkspaceService.GetWorkspacewithworkspaceId: valid-workspace-id?hello, we would let SpiceDB throw an exception, becausecheckPermissionwould fail. This PR fixes that from 2 fronts:INVALID_ARGUMENTa system exception and instead throw aErrorCodes.BAD_REQUEST.WorkspaceServicemethod that needs it using a Regex that seems to have worked for us for ~ 2 years.Related Issue(s)
Fixes ENT-842
How to test
Make a request to some API under
WorkspaceServerwith an invalid workspace ID, you should get back "a valid workspaceId is required"For requests to other resources or things that this validity check just doesn't catch, the error message will be similar to the following:
{"code":"invalid_argument","message":"Invalid request for permission check: Error: 3 INVALID_ARGUMENT: invalid CheckPermissionRequest.Resource: embedded message failed validation | caused by: invalid ObjectReference.ObjectId: value does not match regex pattern \"^(([a-zA-Z0-9/_|\\\\-=+]{1,})|\\\\*)$\""}