ENG-2539: Allow extra fields in JiraTicketSchema secrets#7605
Conversation
The Pydantic schema for jira_ticket secrets was stripping extra fields during validation. This meant PATCH /secret couldn't merge template config (project_key, issue_type, templates) alongside OAuth tokens. Setting extra="allow" lets Fidesplus store arbitrary config in the same secrets dict without requiring fides schema changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis change enables JiraTicketSchema to store additional fields (project_key, issue_type, templates) by allowing extra attributes in the schema, and updates the masking utility to preserve these extra fields when the schema permits additional properties. Tests validate the new behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
set_secrets was assigning a Pydantic model object to the secrets column, which SQLAlchemy's MutableDict rejects. Call model_dump() to convert to a plain dict first. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mask_sensitive_fields was dropping any secret keys not in the schema properties. For schemas with extra="allow" (like JiraTicketSchema), this silently stripped saved data from the GET response even though it was persisted in the DB. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Only preserve fields not in the schema when additionalProperties is true (i.e. the Pydantic model uses extra="allow"). This fixes test failures where non-schema fields like url=None were leaking into responses for standard connection types like Postgres. Schemas like JiraTicketSchema that use extra="allow" will have their additional fields preserved in API responses, while all other connection types maintain the existing behavior of dropping unknown fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fides/api/util/masking_util.py`:
- Around line 40-43: The current branch in masking_util.py that assigns extra
fields when allows_additional is True (new_connection_secrets[key] = value) can
leak sensitive extension data; change it to apply the same masking logic used
for schema fields instead of blindly preserving value — detect sensitive extra
keys (e.g., key.lower().endswith("_secret") or key.lower().endswith("_token"))
and replace them with the masked placeholder, otherwise include the original or
sanitized value; reuse the existing masking helper/function used elsewhere in
this module to ensure consistent behavior for keys handled by allows_additional,
and add a small unit test that verifies keys like "api_secret" and
"access_token" are masked while non-sensitive extras are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2997f51-c800-4189-a5d5-c1ed39147899
📒 Files selected for processing (6)
changelog/7605-jira-ticket-extra-fields.yamlsrc/fides/api/models/messaging.pysrc/fides/api/schemas/connection_configuration/connection_secrets_jira_ticket.pysrc/fides/api/util/masking_util.pytests/ops/schemas/connection_configuration/test_connection_config.pytests/service/test_jira_ticket_connection.py
Greptile SummaryThis PR allows
Confidence Score: 3/5
Important Files Changed
Last reviewed commit: 75a2a4f |
| @@ -27,5 +37,9 @@ def mask_sensitive_fields( | |||
| new_connection_secrets[key] = "**********" | |||
| else: | |||
| new_connection_secrets[key] = value | |||
| elif allows_additional: | |||
| # Preserve non-schema fields only when the schema explicitly | |||
| # allows additional properties (e.g. extra="allow"). | |||
| new_connection_secrets[key] = value | |||
There was a problem hiding this comment.
additionalProperties truthiness may not match Pydantic v2 output
The test for this new branch uses a hand-crafted schema with "additionalProperties": True (Python bool), but Pydantic v2 with extra="allow" generates "additionalProperties": {} (an empty dict) in its model_json_schema() output. In Python bool({}) evaluates to False, so elif allows_additional: would never execute when the schema is obtained via get_connection_type_secret_schema("jira_ticket").
This means the extra fields (like project_key, summary_template) would be silently dropped from the masked secrets in API responses, even though they are preserved in JiraTicketSchema instances and in the database.
The fix should guard against both the boolean and the empty-dict form that Pydantic v2 produces:
raw_additional = secret_schema.get("additionalProperties")
# Pydantic v2 extra="allow" → {} (empty dict, truthy-intended but falsy in Python)
# Pydantic v2 extra="forbid" → False
# hand-crafted schemas → True or False
allows_additional = raw_additional is not False and raw_additional is not NoneAnd an integration test should validate the full pipeline — calling get_connection_type_secret_schema("jira_ticket") and feeding its output into mask_sensitive_fields alongside extra fields — to confirm both halves of the fix compose correctly.
There was a problem hiding this comment.
Verified against the actual runtime: Pydantic 2.12.4 with extra="allow" generates additionalProperties: True (boolean) in JiraTicketSchema.model_json_schema(), not {}. The truthiness check works correctly.
erosselli
left a comment
There was a problem hiding this comment.
approved with some comments
src/fides/api/models/messaging.py
Outdated
| self.secrets = ( | ||
| messaging_secrets.model_dump() | ||
| if hasattr(messaging_secrets, "model_dump") | ||
| else messaging_secrets | ||
| ) |
There was a problem hiding this comment.
Removed from this PR — the real fix is updating the seeding in fidesplus so it doesn't pass model instances directly.
| @@ -27,5 +37,9 @@ def mask_sensitive_fields( | |||
| new_connection_secrets[key] = "**********" | |||
| else: | |||
| new_connection_secrets[key] = value | |||
| elif allows_additional: | |||
| # Preserve non-schema fields only when the schema explicitly | |||
| # allows additional properties (e.g. extra="allow"). | |||
| new_connection_secrets[key] = value | |||
| @@ -0,0 +1,4 @@ | |||
| type: Changed | |||
| description: Allowed extra fields in JiraTicketSchema for Fidesplus extensibility | |||
There was a problem hiding this comment.
instead of "for Fidesplus extensibility" can we say why the extra fields are needed, e/g "for preserving ticket metadata" or whatever
There was a problem hiding this comment.
Updated — made the description more specific about what the extra fields are for.
- Revert unrelated model_dump() change in messaging.py - Update changelog description to be more specific about extra fields purpose Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ticket ENG-2539
Description Of Changes
Allow extra fields in
JiraTicketSchemaso that Fidesplus can store additional configuration (project key, issue type, templates, etc.) in the same secrets dict without requiring schema changes in the OSS repo.Sets
model_config = ConfigDict(extra="allow")onJiraTicketSchemaonly — the baseFidesSchemabehavior is unchanged.Code Changes
src/fides/api/schemas/connection_configuration/connection_secrets_jira_ticket.py- Addmodel_config = ConfigDict(extra="allow")and update docstringtests/service/test_jira_ticket_connection.py- Update test to verify extra fields are preserved (was testing they were ignored)Steps to Confirm
JiraTicketSchemawith extra fields:FidesSchema)Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and worksSummary by CodeRabbit
Release Notes