Update consent server version#3518
Conversation
ThumulaPerera
added
Type/Improvement
breaking change
📝 WalkthroughWalkthroughThe PR updates the default consent client to the v0.3.0 API contract: DTO field names, endpoint paths, headers, query parameters, and element validation behavior change. It also updates consent recording to re-read current purpose definitions before merging approvals, and adds tests for the new filtering behavior. ChangesDefault consent client v0.3.0 adaptation
Consent enforcer stale-element filtering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Kept as a draft as this requires a new release in consent server and a corresponding consent server version bump at ThunderID |
Bundle ReportBundle size has no change ✅ Affected Assets, Files, and Routes:view changes for bundle: console-esmAssets Changed:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
backend/internal/authn/consent/service_test.go (1)
782-786: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a direct failure-path test for
ListConsentPurposes.The new branch that maps purpose-list failures to
ErrorConsentPurposeFetchFailed/InternalServerErroris still uncovered here. A small client-error + server-error pair would lock down the new error handling.Also applies to: 820-822, 855-857
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/authn/consent/service_test.go` around lines 782 - 786, Add direct failure-path coverage for ListConsentPurposes in the consent service tests: the new branch that maps purpose-list errors to ErrorConsentPurposeFetchFailed and InternalServerError is still untested. Extend the relevant test cases around mockConsentSvc.On("ListConsentPurposes", ...) to include one client-error and one server-error scenario, then assert the returned error mapping from the consent service method that calls ListConsentPurposes.backend/internal/consent/default_client.go (1)
51-51: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftVerify the hardcoded revoke actor.
Is this hardcoded brand name intentional?
actionByfeeds the v0.3.0 revoke audit payload, so a fixed"thunder"value makes all revocations indistinguishable. Prefer sourcing this from the caller, runtime config, or a configured service-principal identity.As per path instructions, hardcoded brand-name literals in Go files should be flagged and replaced with a named/runtime value when possible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/consent/default_client.go` at line 51, The hardcoded revoke actor value in default_client.go is making every revoke audit entry look the same. Update the revoke path around revokeActionBy and the revoke audit payload wiring in default_client.go so actionBy comes from the caller, runtime configuration, or a configured service-principal identity instead of a fixed brand-name literal, and keep the constant only if it is truly intended to be a fallback default.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/authn/consent/service.go`:
- Around line 247-265: The merge only filters stale purpose elements when an
existing consent record is found, so first-time consent submissions can still
persist outdated purpose/element decisions from newPurposeItems and misfire
hasEssentialDenial. Update the create path in service.go around the existing
consent handling so createNewConsent uses the same current purpose definitions
fetched via ListConsentPurposes and applies
mergeConsentPurposes/buildPurposeElementSet before saving, reusing the existing
merge/filter logic to sanitize newPurposeItems as well.
In `@backend/internal/consent/default_client_test.go`:
- Around line 481-485: The validateConsentElements test currently uses a broad
HTTP mock that can pass even if the request is made with the empty string
instead of the deduped email target. Tighten the expectation around
validateConsentElements by matching the outgoing request’s query parameter name
to email on the httpMock.EXPECT().Do call, so the test verifies deduping and
empty-skip behavior rather than just any request shape.
In `@backend/internal/consent/default_client.go`:
- Around line 984-993: The Search filter handling in default_client.go is
silently ignoring extra values in filter.PurposeNames by only setting the first
purposeName on the request. Update the consent search path in the relevant
search/filter builder to either reject multi-value PurposeNames explicitly with
an error, or apply the remaining purpose-name constraints client-side after the
service call. Make sure the behavior is implemented where
params.Set("purposeName", ...) is populated so callers never receive results
that violate the requested filter.
- Around line 525-530: The elementNameExists lookup only checks the first page
of search results, so it can incorrectly return false when the match is on a
later page. Update elementNameExists in default_client.go to keep paging through
the consent-element search results using result.Metadata until all pages are
exhausted, returning true as soon as dto.Name matches name and only returning
false after every page has been inspected.
---
Nitpick comments:
In `@backend/internal/authn/consent/service_test.go`:
- Around line 782-786: Add direct failure-path coverage for ListConsentPurposes
in the consent service tests: the new branch that maps purpose-list errors to
ErrorConsentPurposeFetchFailed and InternalServerError is still untested. Extend
the relevant test cases around mockConsentSvc.On("ListConsentPurposes", ...) to
include one client-error and one server-error scenario, then assert the returned
error mapping from the consent service method that calls ListConsentPurposes.
In `@backend/internal/consent/default_client.go`:
- Line 51: The hardcoded revoke actor value in default_client.go is making every
revoke audit entry look the same. Update the revoke path around revokeActionBy
and the revoke audit payload wiring in default_client.go so actionBy comes from
the caller, runtime configuration, or a configured service-principal identity
instead of a fixed brand-name literal, and keep the constant only if it is truly
intended to be a fallback default.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 246d88d4-837f-4311-bdf4-8bfb81ab9630
⛔ Files ignored due to path filters (1)
tests/integration/testutils/mock_consent_server.gois excluded by!**/mock_*.go
📒 Files selected for processing (5)
backend/internal/authn/consent/service.gobackend/internal/authn/consent/service_test.gobackend/internal/consent/default_client.gobackend/internal/consent/default_client_test.gotests/integration/application/consent_api_test.go
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| // revokeActionBy is the value sent in the revoke payload's required actionBy field. The | ||
| // Thunder-internal ConsentRevokeRequest does not carry actor identity yet; once the | ||
| // interface gains an actor field, source it from there instead. | ||
| const revokeActionBy = "thunder" |
There was a problem hiding this comment.
Can we use something else other than thunder? Maybe admin/ user. Btw what happens if we sent this empty as before? Are they rejecting it?
On a separate note we should use thunderid instead of thunder in all places
There was a problem hiding this comment.
what happens if we sent this empty as before? Are they rejecting it? -> this is a required field. Request will get rejected
But currently we do not have any usages of revoke method. We can remove the method from the interface itself. We can add back in the future if needed. WDYT?
There was a problem hiding this comment.
As discussed offline, will keep the revoke method as we have a planned future use
| // element map to the conflict error; everything else falls back to the generic | ||
| // invalid-element-request error. | ||
| func bulkResultErrorToServiceError(msg string) *serviceerror.ServiceError { | ||
| if strings.Contains(msg, "already exists") { |
There was a problem hiding this comment.
Isn't it possible to detect this with a status code? I guess this is the way openfgc returns the error.
There was a problem hiding this comment.
not possible. in bulk element create, response will have a code of 200 and a seperate status for each element. some elements may be created while some may not
There was a problem hiding this comment.
This is not a scalable approach to detect errors right? Shall we create an issue on openfgc side and keep it tracked?
|
|
||
| // v0.3.0 list omits per-purpose elements; fetch each by ID to populate them. Callers | ||
| // (consent prompt building, attribute-purpose sync) compare against Elements, so an | ||
| // empty slice would silently break those flows. |
There was a problem hiding this comment.
Our implementation is done according to the previous openFGC version, but most of these endpoints doesn't have a valid usage on our end.
If this is something not required by us, we can modify the existing data structures to align with the openfgc v0.3.0 models. We don't have to fetch each purpose to align this with the existing model unless there's a valid usage
There was a problem hiding this comment.
This has a valid usage on our end. In all current usages of consent purpose listing, we have a requirement to iterate through elements of those purposes too
| // multiple, send the first and drop the rest — log so the omission is visible. | ||
| if len(filter.PurposeNames) > 0 { | ||
| params.Set("purposeNames", strings.Join(filter.PurposeNames, ",")) | ||
| params.Set("purposeName", filter.PurposeNames[0]) |
There was a problem hiding this comment.
Same as previous comment. If this is something openfgc no longer supports, we can adjust our usages and models to follow their new format. Dropping it silently will not solve the actual problem right?
There was a problem hiding this comment.
Currently we don't have any usages for this field in the filter. Updated the model to match openfgc shape
ThumulaPerera
force-pushed
the
update-consent-server-version
branch
5 times, most recently
from
b5edce5 to
25a186f
Compare
ThumulaPerera
force-pushed
the
update-consent-server-version
branch
from
25a186f to
94dbf89
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/internal/authn/consent/service.go (1)
180-182: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFilter essential-denial checks through the live purpose allowlist.
hasEssentialDenialis computed from the session token beforevalidElementsis built. If an essential element is removed/re-versioned after prompting,mergeConsentPurposesdrops it from persistence, butRecordConsentcan still returnErrorEssentialConsentDenied.Compute essential denials after
buildPurposeElementSet(currentPurposes)and ignore purposes/elements absent fromvalidElements.Also applies to: 209-222
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/authn/consent/service.go` around lines 180 - 182, `RecordConsent` is checking essential denials too early from `sessionData`, which can still trigger `ErrorEssentialConsentDenied` for purposes/elements that were removed or re-versioned. Move the essential-denial check to after `buildPurposeElementSet(currentPurposes)` in `service.go`, and filter `hasEssentialDenials` using `validElements` so only currently allowed purposes/elements can block consent. Also ensure the same live-allowlist filtering is applied in the `mergeConsentPurposes`/persistence flow referenced by the later consent handling block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/internal/authn/consent/service.go`:
- Around line 180-182: `RecordConsent` is checking essential denials too early
from `sessionData`, which can still trigger `ErrorEssentialConsentDenied` for
purposes/elements that were removed or re-versioned. Move the essential-denial
check to after `buildPurposeElementSet(currentPurposes)` in `service.go`, and
filter `hasEssentialDenials` using `validElements` so only currently allowed
purposes/elements can block consent. Also ensure the same live-allowlist
filtering is applied in the `mergeConsentPurposes`/persistence flow referenced
by the later consent handling block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c6c5d7ea-d0e6-486e-b10e-6209d9b62935
📒 Files selected for processing (11)
backend/internal/authn/consent/service.gobackend/internal/authn/consent/service_test.gobackend/internal/consent/ConsentServiceInterface_mock_test.gobackend/internal/consent/consentClientInterface_mock_test.gobackend/internal/consent/default_client.gobackend/internal/consent/default_client_test.gobackend/internal/consent/error_constants.gobackend/internal/consent/interface.gobackend/internal/consent/model.gobackend/internal/consent/service.gobackend/internal/consent/service_test.go
💤 Files with no reviewable changes (9)
- backend/internal/consent/model.go
- backend/internal/consent/error_constants.go
- backend/internal/consent/service.go
- backend/internal/consent/consentClientInterface_mock_test.go
- backend/internal/consent/service_test.go
- backend/internal/consent/interface.go
- backend/internal/consent/ConsentServiceInterface_mock_test.go
- backend/internal/consent/default_client.go
- backend/internal/consent/default_client_test.go
2 participants
Purpose
Update the consent server version to support updating consent perposes via versioning.
🔧 Summary of Breaking Changes
The bundled consent server moves from v0.2.0 to v0.3.0. The DB schema is updated.
Renamed columns (existing tables)
CONSENTCLIENT_IDGROUP_IDCONSENTVALIDITY_TIMEEXPIRATION_TIMEPURPOSECLIENT_IDGROUP_IDPURPOSE_ELEMENT_MAPPINGIS_MANDATORYMANDATORYCONSENT_ELEMENT_APPROVALIS_USER_APPROVEDAPPROVEDRestructured tables — element & purpose are now versioned
ELEMENTandPURPOSEeach row is now one version of the entity. PK isVERSION_ID; logical identity isID(groups versions); a newVERSIONinteger column orders them.ELEMENTgainsNAMESPACE(default'default') andTYPE(default'basic').*_PROPERTYtables (ELEMENT_PROPERTY,PURPOSE_PROPERTY) now key on the version row, not the entity.PURPOSE_ELEMENT_MAPPINGmaps version → version (PURPOSE_VERSION_ID,ELEMENT_VERSION_ID), not entity → entity.CONSENT_ELEMENT_APPROVALand the newPURPOSE_CONSENT_MAPPINGpin each consent to the exact purpose/element versions it was issued against.New tables
CONSENT_STATUS_AUDIT— append-only status history; addsACTION_BY(required by the new revoke API).CONSENT_AUTH_RESOURCE— per-consent authorizations.CONSENT_ATTRIBUTE— per-consent key/value pairs.PURPOSE_CONSENT_MAPPING— links a consent to its purpose version.💥 Impact
🔄 Migration Guide
Drop the old consent DB and recreate it from the v0.3.0 scripts** bundled under
consent/dbscripts/in the new Thunder distribution.Approach
The PR is a wire-format adapter swap — the new consent server (v0.3.0) is bolted on without touching anything outside the consent client.
Approach in five points:
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
Summary
New Features
Bug Fixes
Breaking Changes