feat: granular permissions for Google Drive integration#921
Draft
jeremyeder wants to merge 6 commits intomainfrom
Draft
feat: granular permissions for Google Drive integration#921jeremyeder wants to merge 6 commits intomainfrom
jeremyeder wants to merge 6 commits intomainfrom
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughImplements granular Google Drive permissions by adding OAuth setup with Google Picker-based file selection, storing selected file grants in Kubernetes storage, and managing picker tokens for frontend authentication. Backend handlers manage OAuth flow, file grants, and token lifecycle; frontend components provide OAuth consent UI and file picker interface. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend
participant GoogleOAuth as Google OAuth
participant Backend
participant Storage as Kubernetes<br/>(ConfigMap/Secret)
User->>Frontend: Click "Connect Google Drive"
activate Frontend
Frontend->>Backend: POST /setup (redirectUri, scope)
activate Backend
Backend->>Backend: Generate HMAC-signed state<br/>(projectName, scope, timestamp)
Backend->>Frontend: Return authUrl + state
deactivate Backend
Frontend->>GoogleOAuth: Redirect to authUrl
deactivate Frontend
User->>GoogleOAuth: Consent & authorize
GoogleOAuth->>Frontend: Redirect to callback<br/>with code + state
Frontend->>Backend: GET /callback?code=X&state=Y
activate Backend
Backend->>Backend: Verify HMAC state<br/>Extract scope from state
Backend->>GoogleOAuth: Exchange code for tokens
GoogleOAuth->>Backend: Return access/refresh tokens
Backend->>Storage: SaveIntegration (ConfigMap)<br/>SaveTokens (Secret)
Storage->>Backend: Persisted
Backend->>Frontend: Return integrationID + pickerToken
deactivate Backend
Frontend->>Frontend: Transition to file picker step
sequenceDiagram
actor User
participant Frontend
participant PickerAPI as Google Picker API
participant Backend
participant GoogleDrive as Google Drive API
participant Storage as Kubernetes<br/>(ConfigMap/Secret)
User->>Frontend: Click "Choose Files"
activate Frontend
Frontend->>Backend: GET /picker-token
activate Backend
Backend->>Storage: GetTokens (Secret)
Storage->>Backend: Return tokens + expiresAt
alt Token Expired
Backend->>Backend: Refresh token
Backend->>Storage: UpdateTokens (Secret)
end
Backend->>Frontend: Return accessToken + expiresIn
deactivate Backend
Frontend->>PickerAPI: Initialize Picker<br/>with accessToken
PickerAPI->>Frontend: Picker ready
User->>PickerAPI: Select files/folders
PickerAPI->>Frontend: Return picked files<br/>(id, name, mimeType, etc.)
deactivate Frontend
Frontend->>Frontend: Display FileSelectionSummary
User->>Frontend: Confirm selection
activate Frontend
Frontend->>Backend: PUT /files (files array)
activate Backend
Backend->>Storage: ListFileGrants (current)
Storage->>Backend: Return existing grants
Backend->>Backend: Compare incoming vs existing<br/>Compute added/removed counts<br/>Reactivate matching grants<br/>Create new grants
alt Verify Query Param Set
Backend->>GoogleDrive: Files.Get(id) for each grant
GoogleDrive->>Backend: Success or 401/403
Backend->>Backend: Mark unavailable on API errors
end
Backend->>Storage: UpdateFileGrants (ConfigMap)<br/>SaveIntegration (update FileCount)
Storage->>Backend: Persisted
Backend->>Frontend: Return counts (Added, Removed)<br/>+ full file list
deactivate Backend
Frontend->>Frontend: Mark setup complete
deactivate Frontend
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/backend/handlers/drive_file_grants.go`:
- Around line 61-66: The call to h.storage.ListFileGrants is currently ignoring
errors which can cause loss of existing GrantedAt metadata; change the block
that assigns existingGrants and builds existingByFileID to handle the returned
error from h.storage.ListFileGrants(c.Request.Context(), integration.ID): if an
error occurs, either return the error from the handler (propagate via the
handler's error response) or log the failure with details and abort processing
so you don't treat all files as new; keep the rest of the logic that builds
existingByFileID (iterating over existingGrants and indexing by GoogleFileID)
but only run it when err == nil to preserve GrantedAt timestamps when
ListFileGrants succeeds.
In `@components/backend/handlers/drive_integration_test.go`:
- Around line 91-109: The test currently only checks for an authUrl string;
update the "Should default to granular scope when not specified" test to parse
the JSON response returned by calling HandleDriveSetup (extract the "authUrl"
value from the response written by ginCtx/response recorder), parse its query
parameters and assert the scope parameter includes "drive.file" and does NOT
include "drive" or "drive.readonly"; keep existing assertions
(httpUtils.AssertHTTPStatus, httpUtils.AssertJSONStructure) and replace the
final logger.Log with these explicit scope assertions so the test fails if the
default scope regresses.
- Around line 230-276: Add a new It block under Context("HandlePickerToken")
that saves an expired access token via models.NewDriveIntegration and
storage.SaveTokens (expiresAt := time.Now().UTC().Add(-1 * time.Hour)),
stub/mock the token-refresh path used by HandlePickerToken (the same component
HandlePickerToken calls to exchange refresh tokens for new access tokens) to
return a new access token and updated expiry, call
handler.HandlePickerToken(ginCtx) with the same Gin setup, then assert
httpUtils.AssertHTTPStatus(http.StatusOK),
httpUtils.AssertJSONStructure(["accessToken","expiresIn"]), read the response
with httpUtils.GetResponseJSON and Expect the accessToken equals the refreshed
token and expiresIn > 0, and finally verify storage now persists the refreshed
token (using the existing storage load/inspect helper if available).
- Around line 160-179: The test currently saves an integration for
"default-user" and relies on a fallback when userID is absent; instead remove
the save of a "default-user" integration and change the assertion to expect an
unauthorized response (401 or 403) when calling HandleGetDriveIntegration with
no userID populated. Concretely, update the test that uses
models.NewDriveIntegration("default-user", ...) and storage.SaveIntegration(...)
to not create any integration for the request, call
handler.HandleGetDriveIntegration(ginCtx) unchanged, and replace
httpUtils.AssertHTTPStatus(http.StatusOK) with the appropriate
httpUtils.AssertHTTPStatus(http.StatusUnauthorized or http.StatusForbidden)
assertion to reflect that missing principal must not fall back to
"default-user".
In `@components/backend/handlers/drive_integration.go`:
- Around line 275-283: The code updates integration status via
integration.Disconnect() followed by h.storage.SaveIntegration(...) immediately
before calling h.storage.DeleteIntegration(...), which makes the status update
unobservable; remove the redundant status update and its save (delete the
integration.Disconnect() and the subsequent h.storage.SaveIntegration(...)
call), or alternatively if you intended to persist a disconnected state on
failure, implement explicit partial-failure handling around SaveIntegration and
only call DeleteIntegration when appropriate—refer to the
integration.Disconnect(), SaveIntegration, and DeleteIntegration calls to locate
the lines to change.
- Around line 204-213: The code is populating the wrong struct field when saving
refreshed tokens: instead of setting DriveIntegration.ID it must set
DriveIntegration.UserID because SaveTokens reads integration.UserID to build the
secret name; change the struct literal passed to h.storage.SaveTokens to set
UserID: userID (and keep ProjectName as-is) so refreshed tokens are saved under
the correct key; verify SaveTokens and any callers still expect
integration.UserID.
In `@components/backend/handlers/drive_storage.go`:
- Around line 201-226: The current findConfigMapByIntegrationID function does an
O(n) scan of all drive-integration ConfigMaps; instead add a deterministic
lookup that uses the known configMapName(projectName, userID) to call
s.clientset.CoreV1().ConfigMaps(...).Get(ctx, configMapName(projectName,
userID), metav1.GetOptions{}) and return that ConfigMap (return nil when not
found), and update the handlers to pass projectName and userID into the new
method (e.g., add getConfigMapByProjectAndUser or overload
findConfigMapByIntegrationID to accept projectName,userID) so you avoid listing
and unmarshalling every ConfigMap.
In `@components/backend/handlers/routes.go`:
- Around line 31-36: Currently the code silently generates a random hmacSecret
(via rand.Read into hmacSecret) when DRIVE_HMAC_SECRET / OAUTH_STATE_SECRET are
unset, which breaks state verification across replicas; change this to fail fast
on startup instead: if len(hmacSecret) == 0, log a clear fatal error (e.g.,
using log.Fatalf or equivalent) explaining that DRIVE_HMAC_SECRET and
OAUTH_STATE_SECRET must be provided and then exit, removing the random
generation branch (and the rand.Read call); ensure the error message references
the required env vars so operators can fix configuration.
In `@components/backend/models/drive_test.go`:
- Around line 188-199: Replace the custom substring helpers contains and
containsHelper with the standard library strings.Contains: remove those two
functions, add import "strings", and update test checks (e.g., in
TestFileGrant_Validate) to use strings.Contains(got, tc.errContains) instead of
calling contains; ensure imports are cleaned and tests still compile.
In `@components/backend/routes.go`:
- Around line 17-18: The Drive integration endpoints are being registered on the
public 'api' group but must be registered under the project-scoped route group
with auth/project middleware; update the call to handlers.InitDriveIntegration
to use the project-scoped router/group (the same group that applies project and
authentication middleware) instead of 'api' so setup, callback, picker-token and
file grants are created with project/user context and protection.
In `@components/frontend/src/components/google-picker/file-selection-summary.tsx`:
- Around line 21-28: The FileItem interface's status is too permissive and the
UI only treats "unavailable" as non-active, so revoked grants render as active;
update FileItem.status to a union that includes "revoked" (e.g., 'available' |
'unavailable' | 'revoked' | undefined) and change any active-checks in
file-selection-summary.tsx (and the related 102-106 region) to treat "revoked"
the same as "unavailable" (i.e., consider status === 'available' as the only
active state, everything else non-active) so revoked grants are rendered as
non-active.
In `@components/frontend/src/components/google-picker/google-picker.tsx`:
- Around line 25-26: The prop existingFileIds on the GooglePicker component is
declared but unused; either remove the prop from the GooglePickerProps interface
and all call sites, or implement pre-selection by wiring existingFileIds into
the component: in the GooglePicker (or related hook) add a local selectedIds
state and a useEffect that initializes it from existingFileIds, then apply those
IDs to the picker when building it (e.g., call the picker builder method that
accepts pre-selected IDs or programmatically mark selections before showing);
ensure the prop name existingFileIds is referenced where the picker is
constructed so pre-selected files are reflected in the modify flow.
- Around line 90-97: The mapping that builds selectedFiles incorrectly uses a
falsy check for sizeBytes which turns valid 0 sizes into null; in the
SelectedFile construction (the data.docs.map block that produces selectedFiles)
replace the falsy fallback with a nullish coalescing check so sizeBytes uses
doc.sizeBytes ?? null instead of doc.sizeBytes || null to preserve 0 values.
In `@components/frontend/src/lib/google-picker-loader.ts`:
- Around line 15-30: The current loadScript function resolves immediately if a
matching script tag exists, causing races and stale-tag short-circuits; change
loadScript to keep a module-level Map<string, Promise<void>> of in-flight
promises keyed by src and return the stored promise when present, create and
attach the script only once, resolve the promise only from the script.onload (or
from gapi.load callback where applicable), set pickerApiLoaded/gisLoaded only
inside the actual onload/callback, and on script.onerror reject the promise,
remove the script element and delete the cached promise so retries can start
cleanly (apply the same promise-cache approach to the other script-loading logic
around pickerApiLoaded/gisLoaded).
In `@components/frontend/src/pages/integrations/google-drive/setup.tsx`:
- Around line 55-78: The effect that handles the OAuth callback (the useEffect
reading searchParams and calling driveCallback.mutate) can re-run on page
refresh because the code and state query params remain in the URL; after
successfully exchanging the code you should clear those query params to avoid
re-triggering. Modify the onSuccess handler passed to driveCallback.mutate (the
callback inside the useEffect) to remove code and state from the URL (e.g.,
build a new URL without those search params and call window.history.replaceState
or use your router's navigate/replace) and then call setStep("authenticated");
ensure this runs only on success so errors still surface.
In `@components/frontend/src/services/drive-api.ts`:
- Around line 245-253: disconnectDriveIntegration currently calls handleResponse
which blindly calls response.json(), causing a failure when backend returns 204
No Content; update disconnectDriveIntegration to check for a 204 (or empty body)
on the Response returned by fetch and return an appropriate empty
DisconnectDriveResponse (e.g., {}) instead of passing the response to
handleResponse, otherwise only call handleResponse for non-204 responses so no
empty-body JSON parsing occurs.
In `@specs/001-granular-drive-permissions/contracts/drive-integration-api.yaml`:
- Around line 176-181: The files array schema (items: $ref:
'#/components/schemas/PickerFile') currently lacks an upper bound; add a
maxItems constraint to that files array (e.g., maxItems: 50 or another
project-appropriate limit) so clients cannot submit unbounded arrays and to
match the handler's expected usage patterns and DoS defenses.
- Around line 1-8: The OpenAPI spec lacks security scheme definitions required
by the ValidateProjectContext() middleware; add a components section with a
securitySchemes entry named bearerAuth (type: http, scheme: bearer,
bearerFormat: JWT) and apply it globally (security: - bearerAuth: []) so all
operations require the bearer token; update the top-level document
(openapi/info/description block) to include the new
components.securitySchemes.bearerAuth and the top-level security array.
In `@specs/001-granular-drive-permissions/research.md`:
- Around line 82-88: The "Key Limitations" markdown table violates MD058; add a
single blank line immediately before the table header line starting with "|
Limitation" and a single blank line immediately after the final table row to
separate the table from surrounding paragraphs (ensure the heading "Key
Limitations" remains above the blank line and the following paragraph/content is
below the trailing blank line) so markdownlint-cli2 no longer flags MD058.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c57a3edf-c42e-41d9-804b-2f2639530542
⛔ Files ignored due to path filters (1)
components/backend/go.sumis excluded by!**/*.sum
📒 Files selected for processing (32)
components/backend/go.modcomponents/backend/handlers/drive_file_grants.gocomponents/backend/handlers/drive_file_grants_test.gocomponents/backend/handlers/drive_integration.gocomponents/backend/handlers/drive_integration_test.gocomponents/backend/handlers/drive_storage.gocomponents/backend/handlers/drive_storage_test.gocomponents/backend/handlers/oauth.gocomponents/backend/handlers/routes.gocomponents/backend/models/drive.gocomponents/backend/models/drive_test.gocomponents/backend/routes.gocomponents/backend/tests/constants/labels.gocomponents/frontend/src/components/google-picker/__tests__/file-selection-summary.test.tsxcomponents/frontend/src/components/google-picker/__tests__/google-picker.test.tsxcomponents/frontend/src/components/google-picker/file-selection-summary.tsxcomponents/frontend/src/components/google-picker/google-picker.tsxcomponents/frontend/src/components/ui/alert-dialog.tsxcomponents/frontend/src/lib/__tests__/google-picker-loader.test.tscomponents/frontend/src/lib/google-picker-loader.tscomponents/frontend/src/pages/integrations/google-drive/settings.tsxcomponents/frontend/src/pages/integrations/google-drive/setup.tsxcomponents/frontend/src/services/__tests__/drive-api.test.tscomponents/frontend/src/services/drive-api.tsspecs/001-granular-drive-permissions/checklists/requirements.mdspecs/001-granular-drive-permissions/contracts/drive-integration-api.yamlspecs/001-granular-drive-permissions/data-model.mdspecs/001-granular-drive-permissions/plan.mdspecs/001-granular-drive-permissions/quickstart.mdspecs/001-granular-drive-permissions/research.mdspecs/001-granular-drive-permissions/spec.mdspecs/001-granular-drive-permissions/tasks.md
Comment on lines
+61
to
+66
| // Get existing file grants for comparison | ||
| existingGrants, _ := h.storage.ListFileGrants(c.Request.Context(), integration.ID) | ||
| existingByFileID := make(map[string]models.FileGrant) | ||
| for _, g := range existingGrants { | ||
| existingByFileID[g.GoogleFileID] = g | ||
| } |
Contributor
There was a problem hiding this comment.
Handle error from ListFileGrants to avoid silent metadata loss.
If ListFileGrants fails (e.g., malformed data in storage), the code proceeds with an empty map, treating all files as new and losing existing GrantedAt timestamps. Consider returning an error or logging the failure.
Proposed fix
// Get existing file grants for comparison
- existingGrants, _ := h.storage.ListFileGrants(c.Request.Context(), integration.ID)
+ existingGrants, err := h.storage.ListFileGrants(c.Request.Context(), integration.ID)
+ if err != nil {
+ // Log but continue - treat as empty to allow recovery
+ // log.Printf("warning: failed to list existing grants: %v", err)
+ }
existingByFileID := make(map[string]models.FileGrant)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Get existing file grants for comparison | |
| existingGrants, _ := h.storage.ListFileGrants(c.Request.Context(), integration.ID) | |
| existingByFileID := make(map[string]models.FileGrant) | |
| for _, g := range existingGrants { | |
| existingByFileID[g.GoogleFileID] = g | |
| } | |
| // Get existing file grants for comparison | |
| existingGrants, err := h.storage.ListFileGrants(c.Request.Context(), integration.ID) | |
| if err != nil { | |
| // Log but continue - treat as empty to allow recovery | |
| // log.Printf("warning: failed to list existing grants: %v", err) | |
| } | |
| existingByFileID := make(map[string]models.FileGrant) | |
| for _, g := range existingGrants { | |
| existingByFileID[g.GoogleFileID] = g | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/handlers/drive_file_grants.go` around lines 61 - 66, The
call to h.storage.ListFileGrants is currently ignoring errors which can cause
loss of existing GrantedAt metadata; change the block that assigns
existingGrants and builds existingByFileID to handle the returned error from
h.storage.ListFileGrants(c.Request.Context(), integration.ID): if an error
occurs, either return the error from the handler (propagate via the handler's
error response) or log the failure with details and abort processing so you
don't treat all files as new; keep the rest of the logic that builds
existingByFileID (iterating over existingGrants and indexing by GoogleFileID)
but only run it when err == nil to preserve GrantedAt timestamps when
ListFileGrants succeeds.
Comment on lines
+91
to
+109
| It("Should default to granular scope when not specified", func() { | ||
| // Arrange | ||
| body := map[string]interface{}{ | ||
| "redirectUri": "http://localhost:3000/callback", | ||
| } | ||
| ginCtx := httpUtils.CreateTestGinContext("POST", "/api/projects/test-project/integrations/google-drive/setup", body) | ||
| ginCtx.Params = gin.Params{ | ||
| {Key: "projectName", Value: "test-project"}, | ||
| } | ||
|
|
||
| // Act | ||
| handler.HandleDriveSetup(ginCtx) | ||
|
|
||
| // Assert | ||
| httpUtils.AssertHTTPStatus(http.StatusOK) | ||
| httpUtils.AssertJSONStructure([]string{"authUrl", "state"}) | ||
|
|
||
| logger.Log("HandleDriveSetup defaulted to granular scope") | ||
| }) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Assert the actual OAuth scope in this test.
This only proves the handler returned an auth URL. It would still pass if the default regressed to drive or if broader scopes were appended. Since least-privilege scope selection is the core security contract of this PR, parse the URL and assert the scope query contains drive.file and excludes drive / drive.readonly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/handlers/drive_integration_test.go` around lines 91 - 109,
The test currently only checks for an authUrl string; update the "Should default
to granular scope when not specified" test to parse the JSON response returned
by calling HandleDriveSetup (extract the "authUrl" value from the response
written by ginCtx/response recorder), parse its query parameters and assert the
scope parameter includes "drive.file" and does NOT include "drive" or
"drive.readonly"; keep existing assertions (httpUtils.AssertHTTPStatus,
httpUtils.AssertJSONStructure) and replace the final logger.Log with these
explicit scope assertions so the test fails if the default scope regresses.
Comment on lines
+160
to
+179
| It("Should use default-user when userID is not set", func() { | ||
| // Arrange — save integration for default-user | ||
| integration := models.NewDriveIntegration("default-user", "test-project", models.PermissionScopeGranular) | ||
| err := storage.SaveIntegration(context.Background(), integration) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| ginCtx := httpUtils.CreateTestGinContext("GET", "/api/projects/test-project/integrations/google-drive", nil) | ||
| ginCtx.Params = gin.Params{ | ||
| {Key: "projectName", Value: "test-project"}, | ||
| } | ||
| // Do not set userID — handler should fall back to "default-user" | ||
|
|
||
| // Act | ||
| handler.HandleGetDriveIntegration(ginCtx) | ||
|
|
||
| // Assert | ||
| httpUtils.AssertHTTPStatus(http.StatusOK) | ||
|
|
||
| logger.Log("HandleGetDriveIntegration fell back to default-user") | ||
| }) |
Contributor
There was a problem hiding this comment.
Don't codify a default-user fallback for a missing principal.
This suite otherwise treats integrations as user-scoped. Falling back to "default-user" turns a missing userID into a read of another user's Drive integration, which becomes a cross-tenant leak if auth middleware ever fails to populate the Gin context. The expected result here should be 401/403 instead.
Proposed test expectation
- It("Should use default-user when userID is not set", func() {
- // Arrange — save integration for default-user
- integration := models.NewDriveIntegration("default-user", "test-project", models.PermissionScopeGranular)
- err := storage.SaveIntegration(context.Background(), integration)
- Expect(err).NotTo(HaveOccurred())
-
+ It("Should return unauthorized when userID is not set", func() {
+ // Arrange
ginCtx := httpUtils.CreateTestGinContext("GET", "/api/projects/test-project/integrations/google-drive", nil)
ginCtx.Params = gin.Params{
{Key: "projectName", Value: "test-project"},
}
- // Do not set userID — handler should fall back to "default-user"
+ // Do not set userID — handler should reject the request
// Act
handler.HandleGetDriveIntegration(ginCtx)
// Assert
- httpUtils.AssertHTTPStatus(http.StatusOK)
-
- logger.Log("HandleGetDriveIntegration fell back to default-user")
+ httpUtils.AssertHTTPStatus(http.StatusUnauthorized)
+
+ logger.Log("HandleGetDriveIntegration rejected request without userID")
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| It("Should use default-user when userID is not set", func() { | |
| // Arrange — save integration for default-user | |
| integration := models.NewDriveIntegration("default-user", "test-project", models.PermissionScopeGranular) | |
| err := storage.SaveIntegration(context.Background(), integration) | |
| Expect(err).NotTo(HaveOccurred()) | |
| ginCtx := httpUtils.CreateTestGinContext("GET", "/api/projects/test-project/integrations/google-drive", nil) | |
| ginCtx.Params = gin.Params{ | |
| {Key: "projectName", Value: "test-project"}, | |
| } | |
| // Do not set userID — handler should fall back to "default-user" | |
| // Act | |
| handler.HandleGetDriveIntegration(ginCtx) | |
| // Assert | |
| httpUtils.AssertHTTPStatus(http.StatusOK) | |
| logger.Log("HandleGetDriveIntegration fell back to default-user") | |
| }) | |
| It("Should return unauthorized when userID is not set", func() { | |
| // Arrange | |
| ginCtx := httpUtils.CreateTestGinContext("GET", "/api/projects/test-project/integrations/google-drive", nil) | |
| ginCtx.Params = gin.Params{ | |
| {Key: "projectName", Value: "test-project"}, | |
| } | |
| // Do not set userID — handler should reject the request | |
| // Act | |
| handler.HandleGetDriveIntegration(ginCtx) | |
| // Assert | |
| httpUtils.AssertHTTPStatus(http.StatusUnauthorized) | |
| logger.Log("HandleGetDriveIntegration rejected request without userID") | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/handlers/drive_integration_test.go` around lines 160 -
179, The test currently saves an integration for "default-user" and relies on a
fallback when userID is absent; instead remove the save of a "default-user"
integration and change the assertion to expect an unauthorized response (401 or
403) when calling HandleGetDriveIntegration with no userID populated.
Concretely, update the test that uses models.NewDriveIntegration("default-user",
...) and storage.SaveIntegration(...) to not create any integration for the
request, call handler.HandleGetDriveIntegration(ginCtx) unchanged, and replace
httpUtils.AssertHTTPStatus(http.StatusOK) with the appropriate
httpUtils.AssertHTTPStatus(http.StatusUnauthorized or http.StatusForbidden)
assertion to reflect that missing principal must not fall back to
"default-user".
Comment on lines
+230
to
+276
| Context("HandlePickerToken", func() { | ||
| It("Should return 404 when no tokens exist", func() { | ||
| // Arrange | ||
| ginCtx := httpUtils.CreateTestGinContext("GET", "/api/projects/test-project/integrations/google-drive/picker-token", nil) | ||
| ginCtx.Params = gin.Params{ | ||
| {Key: "projectName", Value: "test-project"}, | ||
| } | ||
| ginCtx.Set("userID", "test-user") | ||
|
|
||
| // Act | ||
| handler.HandlePickerToken(ginCtx) | ||
|
|
||
| // Assert | ||
| httpUtils.AssertHTTPStatus(http.StatusNotFound) | ||
|
|
||
| logger.Log("HandlePickerToken returned 404 for missing tokens") | ||
| }) | ||
|
|
||
| It("Should return 200 with valid non-expired token", func() { | ||
| // Arrange — save tokens that are still valid | ||
| integration := models.NewDriveIntegration("test-user", "test-project", models.PermissionScopeGranular) | ||
| expiresAt := time.Now().UTC().Add(1 * time.Hour) | ||
| err := storage.SaveTokens(context.Background(), integration, "valid-access-token", "refresh-tok", expiresAt) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| ginCtx := httpUtils.CreateTestGinContext("GET", "/api/projects/test-project/integrations/google-drive/picker-token", nil) | ||
| ginCtx.Params = gin.Params{ | ||
| {Key: "projectName", Value: "test-project"}, | ||
| } | ||
| ginCtx.Set("userID", "test-user") | ||
|
|
||
| // Act | ||
| handler.HandlePickerToken(ginCtx) | ||
|
|
||
| // Assert | ||
| httpUtils.AssertHTTPStatus(http.StatusOK) | ||
| httpUtils.AssertJSONStructure([]string{"accessToken", "expiresIn"}) | ||
|
|
||
| var resp map[string]interface{} | ||
| httpUtils.GetResponseJSON(&resp) | ||
| Expect(resp["accessToken"]).To(Equal("valid-access-token")) | ||
| expiresIn := resp["expiresIn"].(float64) | ||
| Expect(expiresIn).To(BeNumerically(">", 0)) | ||
|
|
||
| logger.Log("HandlePickerToken returned valid token") | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Cover the expired-token refresh path.
The picker-token endpoint's new behavior is automatic refresh, but this suite only exercises "missing tokens" and "still valid token" cases. Add a case with an expired access token plus refresh token so regressions do not break picker access after the first hour.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/handlers/drive_integration_test.go` around lines 230 -
276, Add a new It block under Context("HandlePickerToken") that saves an expired
access token via models.NewDriveIntegration and storage.SaveTokens (expiresAt :=
time.Now().UTC().Add(-1 * time.Hour)), stub/mock the token-refresh path used by
HandlePickerToken (the same component HandlePickerToken calls to exchange
refresh tokens for new access tokens) to return a new access token and updated
expiry, call handler.HandlePickerToken(ginCtx) with the same Gin setup, then
assert httpUtils.AssertHTTPStatus(http.StatusOK),
httpUtils.AssertJSONStructure(["accessToken","expiresIn"]), read the response
with httpUtils.GetResponseJSON and Expect the accessToken equals the refreshed
token and expiresIn > 0, and finally verify storage now persists the refreshed
token (using the existing storage load/inspect helper if available).
Comment on lines
+204
to
+213
| accessToken = newToken.AccessToken | ||
|
|
||
| // Update stored tokens | ||
| _ = h.storage.SaveTokens(c.Request.Context(), &models.DriveIntegration{ | ||
| ProjectName: projectName, | ||
| ID: userID, | ||
| }, newToken.AccessToken, newToken.RefreshToken, newToken.Expiry) | ||
|
|
||
| expiresAt = newToken.Expiry | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: Wrong field set in DriveIntegration when saving refreshed tokens.
Line 209 sets ID: userID but SaveTokens uses integration.UserID (not ID) to construct the secret name. This will cause the refreshed tokens to be saved under an incorrect key (using empty UserID), making subsequent token retrievals fail.
🐛 Proposed fix
// Update stored tokens
- _ = h.storage.SaveTokens(c.Request.Context(), &models.DriveIntegration{
- ProjectName: projectName,
- ID: userID,
- }, newToken.AccessToken, newToken.RefreshToken, newToken.Expiry)
+ _ = h.storage.SaveTokens(c.Request.Context(), &models.DriveIntegration{
+ ProjectName: projectName,
+ UserID: userID,
+ }, newToken.AccessToken, newToken.RefreshToken, newToken.Expiry)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| accessToken = newToken.AccessToken | |
| // Update stored tokens | |
| _ = h.storage.SaveTokens(c.Request.Context(), &models.DriveIntegration{ | |
| ProjectName: projectName, | |
| ID: userID, | |
| }, newToken.AccessToken, newToken.RefreshToken, newToken.Expiry) | |
| expiresAt = newToken.Expiry | |
| } | |
| accessToken = newToken.AccessToken | |
| // Update stored tokens | |
| _ = h.storage.SaveTokens(c.Request.Context(), &models.DriveIntegration{ | |
| ProjectName: projectName, | |
| UserID: userID, | |
| }, newToken.AccessToken, newToken.RefreshToken, newToken.Expiry) | |
| expiresAt = newToken.Expiry |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/handlers/drive_integration.go` around lines 204 - 213, The
code is populating the wrong struct field when saving refreshed tokens: instead
of setting DriveIntegration.ID it must set DriveIntegration.UserID because
SaveTokens reads integration.UserID to build the secret name; change the struct
literal passed to h.storage.SaveTokens to set UserID: userID (and keep
ProjectName as-is) so refreshed tokens are saved under the correct key; verify
SaveTokens and any callers still expect integration.UserID.
Comment on lines
+55
to
+78
| useEffect(() => { | ||
| const code = searchParams.get("code"); | ||
| const state = searchParams.get("state"); | ||
|
|
||
| if (code && state) { | ||
| setStep("authenticating"); | ||
| driveCallback.mutate( | ||
| { projectName, code, state }, | ||
| { | ||
| onSuccess: () => { | ||
| setStep("authenticated"); | ||
| }, | ||
| onError: (err) => { | ||
| setError( | ||
| err instanceof Error | ||
| ? err.message | ||
| : "OAuth callback failed. Please try again." | ||
| ); | ||
| setStep("consent"); | ||
| }, | ||
| } | ||
| ); | ||
| } | ||
| }, [searchParams, projectName]); // eslint-disable-line react-hooks/exhaustive-deps |
Contributor
There was a problem hiding this comment.
OAuth callback may re-trigger on page refresh.
After processing the OAuth callback, the code and state query parameters remain in the URL. If the user refreshes the page, the effect will re-run and attempt to exchange an already-used authorization code, causing an error.
Consider clearing the URL parameters after successful callback processing:
🛠️ Suggested fix
driveCallback.mutate(
{ projectName, code, state },
{
onSuccess: () => {
setStep("authenticated");
+ // Clear OAuth params from URL to prevent re-submission on refresh
+ window.history.replaceState({}, '', window.location.pathname);
},
onError: (err) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/pages/integrations/google-drive/setup.tsx` around
lines 55 - 78, The effect that handles the OAuth callback (the useEffect reading
searchParams and calling driveCallback.mutate) can re-run on page refresh
because the code and state query params remain in the URL; after successfully
exchanging the code you should clear those query params to avoid re-triggering.
Modify the onSuccess handler passed to driveCallback.mutate (the callback inside
the useEffect) to remove code and state from the URL (e.g., build a new URL
without those search params and call window.history.replaceState or use your
router's navigate/replace) and then call setStep("authenticated"); ensure this
runs only on success so errors still surface.
Comment on lines
+245
to
+253
| export async function disconnectDriveIntegration( | ||
| projectName: string, | ||
| ): Promise<DisconnectDriveResponse> { | ||
| const response = await fetch(baseUrl(projectName), { | ||
| method: "DELETE", | ||
| }); | ||
|
|
||
| return handleResponse<DisconnectDriveResponse>(response); | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: disconnectDriveIntegration will fail on successful 204 response.
The backend returns 204 No Content (see drive_integration.go:285), but this function calls handleResponse which attempts response.json(). Parsing an empty body throws an error, causing successful disconnects to appear as failures.
🐛 Proposed fix
export async function disconnectDriveIntegration(
projectName: string,
): Promise<DisconnectDriveResponse> {
const response = await fetch(baseUrl(projectName), {
method: "DELETE",
});
- return handleResponse<DisconnectDriveResponse>(response);
+ if (!response.ok) {
+ let body: unknown;
+ try {
+ body = await response.json();
+ } catch {
+ // ignore
+ }
+ const message =
+ typeof body === "object" && body !== null && "message" in body
+ ? String((body as Record<string, unknown>).message)
+ : `Request failed with status ${response.status}`;
+ throw new ApiError(message, response.status, body);
+ }
+
+ // 204 No Content - no body to parse
+ return { success: true };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/services/drive-api.ts` around lines 245 - 253,
disconnectDriveIntegration currently calls handleResponse which blindly calls
response.json(), causing a failure when backend returns 204 No Content; update
disconnectDriveIntegration to check for a 204 (or empty body) on the Response
returned by fetch and return an appropriate empty DisconnectDriveResponse (e.g.,
{}) instead of passing the response to handleResponse, otherwise only call
handleResponse for non-204 responses so no empty-body JSON parsing occurs.
Comment on lines
+1
to
+8
| openapi: 3.1.0 | ||
| info: | ||
| title: Google Drive Granular Permissions API | ||
| version: 1.0.0 | ||
| description: > | ||
| API contracts for managing Google Drive integrations with granular | ||
| file-level permissions via the Google Picker. | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
OpenAPI spec missing security scheme definitions.
All endpoints require authentication (per ValidateProjectContext() middleware), but the spec lacks security scheme definitions. This affects API documentation accuracy and code generators that consume this spec.
🛠️ Suggested fix: Add security schemes
info:
title: Google Drive Granular Permissions API
version: 1.0.0
description: >
API contracts for managing Google Drive integrations with granular
file-level permissions via the Google Picker.
+
+security:
+ - bearerAuth: []And add to components:
components:
securitySchemes:
bearerAuth:
type: http
scheme: bearer
bearerFormat: JWT🧰 Tools
🪛 Checkov (3.2.510)
[high] 1-351: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-351: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/001-granular-drive-permissions/contracts/drive-integration-api.yaml`
around lines 1 - 8, The OpenAPI spec lacks security scheme definitions required
by the ValidateProjectContext() middleware; add a components section with a
securitySchemes entry named bearerAuth (type: http, scheme: bearer,
bearerFormat: JWT) and apply it globally (security: - bearerAuth: []) so all
operations require the bearer token; update the top-level document
(openapi/info/description block) to include the new
components.securitySchemes.bearerAuth and the top-level security array.
Comment on lines
+176
to
+181
| files: | ||
| type: array | ||
| items: | ||
| $ref: '#/components/schemas/PickerFile' | ||
| minItems: 1 | ||
| description: Files selected via the Google Picker. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding maxItems constraint for the files array.
Without a maximum, clients could submit unbounded arrays. Adding a reasonable limit provides defense-in-depth against DoS and aligns with the handler's expected usage patterns.
files:
type: array
items:
$ref: '#/components/schemas/PickerFile'
minItems: 1
+ maxItems: 1000
description: Files selected via the Google Picker.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| files: | |
| type: array | |
| items: | |
| $ref: '#/components/schemas/PickerFile' | |
| minItems: 1 | |
| description: Files selected via the Google Picker. | |
| files: | |
| type: array | |
| items: | |
| $ref: '#/components/schemas/PickerFile' | |
| minItems: 1 | |
| maxItems: 1000 | |
| description: Files selected via the Google Picker. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/001-granular-drive-permissions/contracts/drive-integration-api.yaml`
around lines 176 - 181, The files array schema (items: $ref:
'#/components/schemas/PickerFile') currently lacks an upper bound; add a
maxItems constraint to that files array (e.g., maxItems: 50 or another
project-appropriate limit) so clients cannot submit unbounded arrays and to
match the handler's expected usage patterns and DoS defenses.
Comment on lines
+82
to
+88
| **Key Limitations**: | ||
| | Limitation | Mitigation | | ||
| | ---------- | ---------- | | ||
| | Client-side only (JavaScript iframe) | Use Drive API with stored file IDs for server-side operations after initial selection | | ||
| | No programmatic file selection | Store file IDs after first selection; only show Picker for new grants | | ||
| | Folder selection is shallow (no recursive access with `drive.file`) | Users must select individual files within folders, or select the folder and document this limitation | | ||
| | Mobile UX is responsive iframe, not native | Acceptable for initial release; consider deep-linking to Drive app later | |
Contributor
There was a problem hiding this comment.
Fix the table spacing so markdown lint passes.
markdownlint-cli2 is already flagging MD058 here; add a blank line before and after the table.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 83-83: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/001-granular-drive-permissions/research.md` around lines 82 - 88, The
"Key Limitations" markdown table violates MD058; add a single blank line
immediately before the table header line starting with "| Limitation" and a
single blank line immediately after the final table row to separate the table
from surrounding paragraphs (ensure the heading "Key Limitations" remains above
the blank line and the following paragraph/content is below the trailing blank
line) so markdownlint-cli2 no longer flags MD058.
Add file-level (drive.file) permissions for Google Drive integration, replacing the default full-drive access scope. Users select specific files via the Google Picker instead of granting access to all Drive files. Backend: - Add DriveIntegration and FileGrant models with state machines - Add K8s ConfigMap/Secret-backed storage for integrations and tokens - Add OAuth scope constants and GetGoogleDriveScopes() helper - Add drive integration handlers (setup, callback, picker-token, get, disconnect) - Add file grant handlers (list, update with add/remove counting) - Add route registration with Unleash feature flag gating - Fix nil-pointer dereference in GetIntegration not-found path Frontend: - Add drive-api.ts service with React Query hooks for all endpoints - Add GooglePicker component wrapping Google Picker API - Add FileSelectionSummary component with mime-type icons - Add google-picker-loader for async script loading - Add alert-dialog Shadcn UI component - Add setup and settings pages for Drive integration Specs: - Add speckit artifacts (spec, plan, tasks, research, data-model, contracts, checklists, quickstart) Tests (80 total): - Backend: 40 tests (models, storage, integration, file grants) - Frontend: 35 tests (API client, components, loader) + 5 loader tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Register DriveIntegrationHandler and DriveFileGrantsHandler in the backend's route setup via InitDriveIntegration(). Constructs handlers from GOOGLE_OAUTH_CLIENT_ID/SECRET env vars, creates DriveStorage backed by the backend's K8s client, and registers routes under /api/projects/:projectName/integrations/google-drive/. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix TypeScript error: null-check searchParams in project page - Fix critical bug: use UserID instead of ID when saving refreshed tokens - Fix disconnect API: handle 204 No Content without parsing JSON body - Handle ListFileGrants errors with warning log instead of silently ignoring - Improve HMAC secret warning: log.Fatalf on generation failure, warn about multi-replica implications when using random secret - Remove redundant Disconnect() + SaveIntegration before DeleteIntegration - Fix sizeBytes falsy check: use ?? instead of || to preserve 0 values - Narrow FileItem status type to union of valid states - Render revoked file grants as non-active with "Revoked" badge - Clear OAuth params from URL after successful callback to prevent re-submission Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jeremyeder
force-pushed
the
feat/granular-drive-permissions-918
branch
from
0bcb160 to
59101dc
Compare
…lure - Fix params possibly null in scheduled session pages - Fix searchParams possibly null in Drive setup page - Move google-drive setup/settings from src/pages/ to src/components/ to prevent Next.js Pages Router prerendering (they are prop-based components, not standalone pages) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Extract getUserID() helper to eliminate 8 duplicate userID fallback blocks - Extract assertOk() in drive-api.ts to DRY up error handling - Use IntegrationStatus type for statusLabels instead of plain string keys Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Proceeding with an empty existing-grants map when the fetch fails causes all files to be treated as new additions, producing an incorrect diff. Return 500 so callers see the real error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Implements file-level (
drive.file) permissions for Google Drive integration (#918), replacing the default full-drive access scope. Users select specific files via the Google Picker instead of granting access to all Drive files.drive.file, new handlers for integration lifecycle and file grant CRUD, K8s ConfigMap/Secret storage, Unleash feature flag gatingKey changes
components/backend/models/drive.go— DriveIntegration and FileGrant models with state machinescomponents/backend/handlers/drive_*.go— integration, file grants, storage, routescomponents/backend/handlers/oauth.go— scope constants andGetGoogleDriveScopes()helpercomponents/frontend/src/services/drive-api.ts— API client with React Query hookscomponents/frontend/src/components/google-picker/— Picker and file summary componentscomponents/frontend/src/pages/integrations/google-drive/— setup and settings pagesspecs/001-granular-drive-permissions/— speckit artifactsTest plan
Closes #918
🤖 Generated with Claude Code