Feat/drive interop save attachment #408

jbpenrath · 2025-11-04T08:57:03Z

Purpose

Install django-lasuite and our custom authentication backend to inherit
from the LaSuiteOIDCAuthenticationBackend

Setup Drive resource server query to retrieve and save files from a configured
Drive instance through OIDC resource server.

CleanShot.2025-11-04.at.09.58.25.mp4

Summary by CodeRabbit

New Features
- Save message attachments to your personal Drive workspace and preview Drive files directly from messages (upload & preview UI).
Bug Fixes
- Improved attachment display and sizing within message bodies.
Documentation
- Updated configuration docs with new Drive and OIDC environment settings.
Chores
- Authentication flow and session token handling updated; OIDC-related settings renamed and new token storage options added.

coderabbitai · 2025-11-04T08:57:25Z

Walkthrough

Migrates OIDC from mozilla-django-oidc to django-lasuite, adds an interop Drive API (GET/POST) with backend handling and OpenAPI schemas, introduces frontend Drive upload/preview components, renames and adds OIDC/session config, and updates tests and locales accordingly.

Changes

Cohort / File(s)	Summary
OIDC auth migration `src/backend/core/authentication/backends.py`, `src/backend/core/authentication/urls.py`, `src/backend/core/authentication/views.py`, `src/backend/core/tests/authentication/test_backends.py`, `src/backend/core/tests/authentication/test_urls.py`, `src/backend/core/tests/authentication/test_views.py`	Replace mozilla-django-oidc with django-lasuite; new backend API (get_or_create_user, post_get_or_create_user, helpers); remove custom logout views/routes; update/remove related tests.
Drive API (backend) `src/backend/core/api/viewsets/drive.py`, `src/backend/core/api/utils.py`, `src/backend/core/urls.py`	Add DriveAPIView (GET list, POST upload) that proxies to external Drive service with OIDC token refresh; add get_attachment_from_blob_id utility; register route under /api/v{version}/interop/drive/.
OpenAPI / serializers `src/backend/core/api/openapi.json`, `src/backend/core/api/serializers.py`	Add DriveUploadAttachmentRequest, PaginatedDriveItemResponse, PartialDriveItem schemas; remove `id` from Attachment representation; add PartialDriveItemSerializer.
Config & settings `src/backend/core/api/viewsets/config.py`, `src/backend/messages/settings.py`, `docs/env.md`, `src/backend/core/tests/api/test_config.py`	Add DRIVE.file_url, include file_url in runtime response; rename USER_OIDC_* env vars to OIDC_USERINFO_*; add OIDC_STORE_ACCESS_TOKEN, OIDC_STORE_REFRESH_TOKEN, OIDC_STORE_REFRESH_TOKEN_KEY; update docs and tests.
Backend tests (Drive) `src/backend/core/tests/api/test_drive.py`	New comprehensive tests for Drive GET/POST flows, auth/token refresh handling, blob resolution, upload-to-presigned-url flow, and error cases.
Dependencies `src/backend/pyproject.toml`	Remove mozilla-django-oidc, add `django-lasuite[all]==0.0.14`.
Frontend — Drive UI `src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-button.tsx`, `.../drive-preview-link.tsx`, `.../attachment-item.tsx`, `.../_index.scss`	Add DriveUploadButton (state machine, caching, upload mutation) and DrivePreviewLink; update AttachmentItem to conditionally render upload/preview and accept canDownload; add tooltip/container styles.
Frontend — attachment types & uploader `src/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsx`, `.../attachment-uploader.tsx`	Extend DriveFile type to include `id`; tighten id-matching logic in attachment removal; pass canDownload prop to AttachmentItem.
Frontend — locales & minor tweaks `src/frontend/public/locales/common/en-US.json`, `src/frontend/public/locales/common/fr-FR.json`, `src/frontend/src/features/forms/components/message-composer/index.tsx`, `src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx`	Add Drive-related translation keys (EN/FR); remove a comment in composer; constrain iframe images to max-width:100vw.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Frontend
    participant Backend
    participant Drive
    participant S3

    rect rgb(210,230,255)
    Note over User,Drive: Drive listing flow
    User->>Frontend: open Drive picker / request list
    Frontend->>Backend: GET /api/v1.0/interop/drive/?title=...
    Backend->>Backend: ensure OIDC access token (refresh if needed)
    Backend->>Drive: GET /workspaces (Authorization: Bearer ...)
    Drive-->>Backend: list workspaces
    Backend->>Drive: GET /workspaces/{main}/children?title=...
    Drive-->>Backend: paginated file list
    Backend-->>Frontend: PaginatedDriveItemResponse
    end

sequenceDiagram
    participant User
    participant Frontend
    participant Backend
    participant Drive
    participant S3

    rect rgb(220,255,220)
    Note over User,S3: Drive upload flow
    User->>Frontend: click upload (blob_id)
    Frontend->>Backend: POST /api/v1.0/interop/drive/ { blob_id }
    Backend->>Backend: resolve blob_id -> Attachment (access checks)
    Backend->>Drive: POST /files (create file entry)
    Drive-->>Backend: { id, presigned_upload_url, upload_end_url }
    Backend->>S3: PUT to presigned_upload_url (attachment bytes)
    S3-->>Backend: 200 OK
    Backend->>Drive: POST upload-ended callback
    Drive-->>Backend: file metadata (PartialDriveItem)
    Backend-->>Frontend: PartialDriveItem
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

Review focus:
- OIDC migration: backend hooks, token storage/encryption, removed logout paths.
- Drive API: token refresh usage, blob-id decoding, access checks, external request error handling.
- Tests: ensure mocks align with external interactions.
- Frontend: DriveUploadButton state machine, caching, and cache updates after upload.
- Schema change: Attachment.id removal impact.

Possibly related PRs

✨(images) implement cid: image support #378 — touches AttachmentSerializer/openapi Attachment fields; likely overlaps schema changes.
Ci/push fr translations #367 — related frontend i18n updates; may overlap locale changes.
Front/fold message quotes #382 — touches message-body/thread-view frontend components; may intersect UI changes.

Suggested reviewers

sylvinus
sdemagny

Poem

🐰 I found a token, neat and spry,

Lasuite helped it fly on high.
Files hop in with one small click,
Previews blink and uploads stick.
A carrot-toast to code — hop quick!

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main feature being implemented: Drive interoperability for saving attachments to a Drive workspace.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch feat/drive-interop-save-attachment

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Pylint (4.0.2)

src/backend/core/api/serializers.py

src/backend/core/api/utils.py

src/backend/core/api/viewsets/config.py

9 others

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Insatll django-lasuite and our custom authentication backend to inherit from the LaSuiteOIDCAuthenticationBackend

Setup Drive resource server query to retrieve and save files from a configured Drive instance through OIDC resource server. suitenumerique/drive#379

coderabbitai

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/backend/core/api/serializers.py (1)

374-399: Verify that frontend code will break due to attachment id field removal.

The search confirms this is indeed a breaking change:

Frontend code uses attachment.id in two locations:

src/frontend/src/features/utils/mail-helper.tsx:163 - uses a.id in data-id attribute

src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsx:87 - uses attachment.id as fileId parameter to DrivePreviewLink

Current serializer does not expose id:

The AttachmentSerializer.Meta.fields list (lines 374-399) does not include "id"

Only exposes: blobId, name, size, type, sha256, created_at, cid

Required actions before merge:

Update frontend code in both locations to use an alternative identifier (likely blobId instead of id)

Ensure API consumers are aligned with this change

Consider deprecation approach if this is a public API

src/backend/core/authentication/backends.py (1)

34-86: Fix two critical method call errors in get_or_create_user (lines 51, 78)

Line 51: get_userinfo(access_token, id_token, payload) passes wrong parameter. Parent method signature expects get_userinfo(self, access_token, id_token=None, userinfo_endpoint=None). Replace payload with userinfo_endpoint or remove the third argument if not needed.

Line 78: self.update_user_if_needed(user, claims) calls undefined method. Parent class provides update_user(self, user, claims), not update_user_if_needed. Either rename to update_user or define the custom method in this class.

🧹 Nitpick comments (13)

src/frontend/public/locales/common/en-US.json (1)

54-55: Minor English phrasing suggestions for improved clarity.

The translations are complete and properly formatted. The French equivalents are high-quality. However, two English strings could use slightly more natural phrasing:

Line 251: "Save into your Drive's workspace" → Consider "Save to your Drive's workspace" (more idiomatic)

Lines 54-55: The passive constructions are acceptable but could be tightened if consistency with other UI messages suggests an active voice style

These are minor stylistic suggestions; the current phrasing is acceptable and matches the PR's localization additions.

Also applies to: 226-226, 251-251
src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/_index.scss (1)
42-42: Remove commented-out code.

If the gap is no longer needed for the attachment actions layout, remove the commented line rather than leaving it in the codebase.

Apply this diff if the gap is definitively not needed:
 .attachment-item-actions {
     display: flex;
     flex-direction: row;
-    // gap: var(--c--theme--spacings--2xs);
     flex-wrap: nowrap;
src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-button.tsx (1)
86-92: Consider extracting timeout constants.

The hardcoded timeout values (1500ms for success, 5000ms for error) could be defined as named constants to improve maintainability and make the durations more discoverable.

For example, at the top of the file:
const SUCCESS_TIMEOUT_MS = 1500;
const ERROR_TIMEOUT_MS = 5000;
Then use:
-            }, state === 'success' ? 1500 : 5000);
+            }, state === 'success' ? SUCCESS_TIMEOUT_MS : ERROR_TIMEOUT_MS);
docs/env.md (1)
157-162: Good documentation additions, but consider clarifying security requirements.

The new OIDC configuration variables are well-documented and follow the existing format. However, consider enhancing the documentation:
OIDC_STORE_REFRESH_TOKEN_KEY - While the note about Fernet key is helpful, consider adding:

A note that this becomes required when OIDC_STORE_REFRESH_TOKEN is True

A reference to how to generate a valid Fernet key

A security warning about key rotation and storage
Consider adding an example or reference link for generating a Fernet key, such as:
from cryptography.fernet import Fernet
key = Fernet.generate_key()
src/backend/core/api/openapi.json (5)
186-196: Add URI format for DRIVE URLs.

file_url is a URL; declare "format": "uri" for validation. Consider adding the same to sdk_url and api_url for consistency.
-                                                "file_url": {
-                                                    "type": "string",
-                                                    "readOnly": true
-                                                }
+                                                "file_url": {
+                                                    "type": "string",
+                                                    "format": "uri",
+                                                    "readOnly": true
+                                                }
1660-1735: Interop Drive: fix tag, status code, and basic ergonomics.

Tags: GET uses interop/drive, POST uses interop. Use one tag for both.

Status: POST should return 201 for create.

Pagination: Response is paginated but no page (or equivalent) query param.

Errors: Add 400/403/404 like other endpoints.

Param naming: consider q (optional) to align with search endpoints elsewhere; if staying with title, consider not required.
-            "get": {
+            "get": {
               "operationId": "interop_drive_retrieve",
               "description": "Search for files created by the current user.",
               "parameters": [
+                {
+                  "name": "page",
+                  "in": "query",
+                  "required": false,
+                  "schema": { "type": "integer" },
+                  "description": "A page number within the paginated result set."
+                },
                 {
                   "in": "query",
                   "name": "title",
                   "schema": {
                     "type": "string"
                   },
                   "description": "Search files by title.",
-                  "required": true
+                  "required": true
                 }
               ],
-              "tags": [ "interop/drive" ],
+              "tags": [ "interop/drive" ],
               "security": [ { "cookieAuth": [] } ],
               "responses": {
                 "200": { ... },
+                "400": { "description": "Bad request" },
+                "403": { "description": "Permission denied" },
+                "404": { "description": "Not found" }
               }
             },
             "post": {
               "operationId": "interop_drive_create",
               "description": "Create a new file in the main workspace.",
-              "tags": [ "interop" ],
+              "tags": [ "interop/drive" ],
               "requestBody": { ... },
               "security": [ { "cookieAuth": [] } ],
               "responses": {
-                "200": {
+                "201": {
                   "content": {
                     "application/json": {
                       "schema": { "$ref": "#/components/schemas/PartialDriveItem" }
                     }
                   },
                   "description": "File created successfully"
-                }
+                },
+                "400": { "description": "Bad request" },
+                "403": { "description": "Permission denied" },
+                "404": { "description": "Not found" }
               }
             }
5302-5314: Unify JSON field casing and wording (blob_id → blobId).

API elsewhere uses camelCase (blobId, messageId). Rename for consistency and clarify description (“blob” vs “attachment”).
-            "DriveUploadAttachmentRequest": {
+            "DriveUploadAttachmentRequest": {
               "type": "object",
               "properties": {
-                "blob_id": {
+                "blobId": {
                   "type": "string",
                   "format": "uuid",
-                  "description": "ID of the attachment to upload"
+                  "description": "ID of the blob to upload"
                 }
               },
               "required": [
-                "blob_id"
+                "blobId"
               ]
             },
If this API is already consumed, consider supporting both names temporarily and deprecating blob_id.

6600-6627: Align pagination schema with existing Paginated types.*

Don’t require next/previous (they can be absent/null). Other paginated schemas only require count and results.

Add "format": "uri" to next/previous.
-            "PaginatedDriveItemResponse": {
+            "PaginatedDriveItemResponse": {
               "type": "object",
               "properties": {
                 "count": { "type": "integer" },
-                "next": { "type": "string", "nullable": true },
-                "previous": { "type": "string", "nullable": true },
+                "next": { "type": "string", "format": "uri", "nullable": true },
+                "previous": { "type": "string", "format": "uri", "nullable": true },
                 "results": {
                   "type": "array",
                   "items": { "$ref": "#/components/schemas/PartialDriveItem" }
                 }
               },
               "required": [
-                "count","next","previous","results"
+                "count","results"
               ]
             },
6814-6838: Verify Drive item ID format; relax if not guaranteed UUID.

External Drive IDs are often opaque strings. If not guaranteed UUID, drop "format": "uuid" to avoid client-side validation failures.
-          "id": { "type": "string", "format": "uuid" },
+          "id": { "type": "string" },
src/backend/core/authentication/backends.py (4)
50-85: Consider adding logging for authentication events

Per coding guidelines, log expected and unexpected actions with appropriate log levels. Consider adding logging for:

User creation (line 81-82)

Existing user authentication (line 75)

Authentication failures (lines 54-55, 59-61)

Example:
         elif self.should_create_user(email):
             user = self.create_user(claims)
             _user_created = True
+            logger.info("Created new user with sub=%s", sub)
As per coding guidelines

50-85: The _user_created flag appears unused in the post-hook

The _user_created flag is passed to post_get_or_create_user but that method doesn't use it (line 87-90). Consider either using it to conditionally perform setup actions, or removing it if unnecessary.

87-90: Unused parameters in post-hook

The claims and _user_created parameters are not used in this method. If they're intended for future use or subclass overrides, consider documenting this in the docstring. Otherwise, they could be removed to reduce confusion.

98-103: Good error handling for duplicate email scenario

The method correctly wraps the DuplicateEmailError as a SuspiciousOperation and properly chains the exception. Consider adding a log statement before raising the exception to track these security-relevant events.

As per coding guidelines
     def get_existing_user(self, sub, email):
         """Get an existing user by sub or email."""
         try:
             return User.objects.get_user_by_sub_or_email(sub, email)
         except DuplicateEmailError as err:
+            logger.warning("Duplicate email detected: %s", err.message)
             raise SuspiciousOperation(err.message) from err

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between afcced7 and bf29028.

⛔ Files ignored due to path filters (11)

src/backend/poetry.lock is excluded by !**/*.lock
src/frontend/src/features/api/gen/index.ts is excluded by !**/gen/**
src/frontend/src/features/api/gen/interop-drive/interop-drive.ts is excluded by !**/gen/**
src/frontend/src/features/api/gen/interop/interop.ts is excluded by !**/gen/**
src/frontend/src/features/api/gen/models/attachment.ts is excluded by !**/gen/**
src/frontend/src/features/api/gen/models/config_retrieve200_driv_e.ts is excluded by !**/gen/**
src/frontend/src/features/api/gen/models/drive_upload_attachment_request.ts is excluded by !**/gen/**
src/frontend/src/features/api/gen/models/index.ts is excluded by !**/gen/**
src/frontend/src/features/api/gen/models/interop_drive_retrieve_params.ts is excluded by !**/gen/**
src/frontend/src/features/api/gen/models/paginated_drive_item_response.ts is excluded by !**/gen/**
src/frontend/src/features/api/gen/models/partial_drive_item.ts is excluded by !**/gen/**

📒 Files selected for processing (28)

docs/env.md (1 hunks)
src/backend/core/api/openapi.json (5 hunks)
src/backend/core/api/serializers.py (1 hunks)
src/backend/core/api/utils.py (2 hunks)
src/backend/core/api/viewsets/blob.py (2 hunks)
src/backend/core/api/viewsets/config.py (2 hunks)
src/backend/core/api/viewsets/drive.py (1 hunks)
src/backend/core/authentication/backends.py (3 hunks)
src/backend/core/authentication/urls.py (1 hunks)
src/backend/core/authentication/views.py (0 hunks)
src/backend/core/tests/api/test_config.py (2 hunks)
src/backend/core/tests/api/test_drive.py (1 hunks)
src/backend/core/tests/authentication/test_backends.py (8 hunks)
src/backend/core/tests/authentication/test_urls.py (0 hunks)
src/backend/core/tests/authentication/test_views.py (0 hunks)
src/backend/core/urls.py (2 hunks)
src/backend/messages/settings.py (2 hunks)
src/backend/pyproject.toml (1 hunks)
src/frontend/public/locales/common/en-US.json (3 hunks)
src/frontend/public/locales/common/fr-FR.json (3 hunks)
src/frontend/src/features/forms/components/message-composer/index.tsx (0 hunks)
src/frontend/src/features/forms/components/message-form/attachment-uploader.tsx (2 hunks)
src/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsx (1 hunks)
src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/_index.scss (3 hunks)
src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsx (3 hunks)
src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-preview-link.tsx (1 hunks)
src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-button.tsx (1 hunks)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/message-body.tsx (1 hunks)

💤 Files with no reviewable changes (4)

src/frontend/src/features/forms/components/message-composer/index.tsx
src/backend/core/tests/authentication/test_urls.py
src/backend/core/authentication/views.py
src/backend/core/tests/authentication/test_views.py

🧰 Additional context used

📓 Path-based instructions (6)

src/backend/**/*.py