-
Notifications
You must be signed in to change notification settings - Fork 18
Authorization Model Metadata #228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…yle labels - Add authorization_model_metadata map field to AuthorizationModel message - Add authorization_model_metadata map field to WriteAuthorizationModelRequest message - Support up to 20 key-value pairs with Kubernetes-compatible DNS-1123 validation - Key validation: lowercase alphanumeric with dashes/dots, max 63 chars - Value validation: max 256 chars - Enables model categorization, environment tagging, and operational labels This enables users to add metadata like: { "environment": "production", "team": "platform", "version": "v1.2.3" }
- Add metadata field to AuthorizationModel message in authzmodel.proto - Add metadata field to WriteAuthorizationModelRequest in openfga_service.proto - Support for Kubernetes-compatible DNS-1123 validation for metadata keys - Maximum 20 metadata pairs per authorization model - Key length limit: 63 bytes, Value length limit: 256 bytes - Update generated protobuf code with proper JSON serialization - Update OpenAPI documentation with metadata field examples This enables labeling and categorization of authorization models for better organization, versioning, and operational management. Closes: #XXX
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API Server
participant Validation
participant Storage
Client->>API Server: WriteAuthorizationModelRequest (includes metadata)
API Server->>Validation: Validate metadata (keys, values, count)
Validation-->>API Server: Validation result
API Server->>Storage: Store AuthorizationModel with metadata
Storage-->>API Server: Store result
API Server-->>Client: WriteAuthorizationModelResponse (with error if validation/storage fails)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
docs/openapiv2/apidocs.swagger.json (1)
1-2
:buf format
failure blocks CI – re-run formatter and commitThe pipeline reports uncommitted formatting changes.
Run the project’s formatter before merging:buf format -w docs/openapiv2/apidocs.swagger.json git add docs/openapiv2/apidocs.swagger.json git commit -m "chore: buf format apidocs.swagger.json"
♻️ Duplicate comments (1)
docs/openapiv2/apidocs.swagger.json (1)
177-183
: Same “default” error response pattern repeatedEarlier feedback (lines 85-91) applies; no further comments required on these repetitions.
Also applies to: 254-259, 278-289, 436-441, 490-495, 590-596, 878-884, 980-986, 1082-1087, 1192-1198, 1294-1299, 1412-1417
🧹 Nitpick comments (2)
openfga/v1/authzmodel.proto (1)
53-59
:buf format
failure – run the formatter before committingCI flagged these lines.
buf format
will normalise indentation and trailing commas so the style check passes.openfga/v1/openfga_service.proto (1)
1551-1557
: Formatting mismatch triggersbuf format
errorAs with the previous file, run
buf format
to align indentation and option-list commas so CI passes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
proto/openfga/v1/authzmodel.pb.go
is excluded by!**/*.pb.go
proto/openfga/v1/openfga_service.pb.go
is excluded by!**/*.pb.go
📒 Files selected for processing (5)
docs/openapiv2/apidocs.swagger.json
(28 hunks)openfga/v1/authzmodel.proto
(1 hunks)openfga/v1/openfga_service.proto
(1 hunks)proto/openfga/v1/authzmodel.pb.validate.go
(2 hunks)proto/openfga/v1/openfga_service.pb.validate.go
(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: review
openfga/v1/authzmodel.proto
[error] 53-61: buf format check failed. The formatting differs from the expected style. Run 'buf format' to fix code style issues.
openfga/v1/openfga_service.proto
[error] 1551-1559: buf format check failed. The formatting differs from the expected style. Run 'buf format' to fix code style issues.
docs/openapiv2/apidocs.swagger.json
[error] 1-1: Make step failed due to uncommitted changes detected in docs/openapiv2/apidocs.swagger.json. Please run 'buf format -w' and commit the changes.
🔇 Additional comments (5)
proto/openfga/v1/openfga_service.pb.validate.go (2)
4476-4541
: LGTM! Well-structured metadata validation logic.The validation implementation correctly enforces all the specified constraints:
- Maximum 20 metadata key-value pairs
- Non-empty keys with DNS-1123 naming convention
- 63-byte key length limit
- 256-byte value length limit (empty values allowed)
The code follows the established patterns in the file and provides clear error messages for validation failures.
4634-4634
: LGTM! Correct DNS-1123 regex pattern for metadata keys.The regex pattern
^[a-z0-9]([a-z0-9\\-\\.]*[a-z0-9])?$
properly enforces Kubernetes-compatible DNS-1123 naming conventions:
- Starts and ends with alphanumeric characters
- Allows lowercase letters, numbers, hyphens, and dots
- Ensures proper subdomain naming format
This aligns perfectly with the PR requirements for metadata key validation.
proto/openfga/v1/authzmodel.pb.validate.go (1)
335-335
: LGTM! DNS-1123 regex pattern is correctly implemented.The regex pattern correctly enforces DNS-1123 naming conventions for metadata keys, ensuring they start and end with alphanumeric characters and can contain lowercase letters, numbers, hyphens, and dots in between.
docs/openapiv2/apidocs.swagger.json (2)
85-91
: 👍 Adding a “default” error envelope improves client resilienceThe generic fallback response referencing
#/definitions/Status
is welcome and maintains parity across endpoints.
No action required.
124-129
: Confirm server emits 200 vs 201 forCreateStore
POST /stores
now documents both 200 and 201.
API conventions typically reserve 201 for successful creates; 200 is unusual unless the server performs an idempotent upsert or redirect.Please verify the server implementation to avoid misleading clients.
"metadata": { | ||
"type": "object", | ||
"example": { | ||
"environment": "production", | ||
"team": "platform", | ||
"version": "v1.2.3" | ||
}, | ||
"additionalProperties": { | ||
"type": "string" | ||
}, | ||
"title": "Labels and metadata for the authorization model (similar to Kubernetes labels)" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Expose metadata validation constraints in the WriteAuthorizationModel schema
The protobuf layer enforces:
• ≤20 pairs
• DNS-1123 key pattern (≤63 bytes, non-empty)
• value ≤256 bytes
Expose the same in OpenAPI so client SDKs may validate early:
"metadata": {
"type": "object",
+ "maxProperties": 20,
+ "propertyNames": {
+ "type": "string",
+ "pattern": "^[a-z0-9]([-a-z0-9\\.]{0,61}[a-z0-9])?$",
+ "minLength": 1,
+ "maxLength": 63
+ },
"example": {
"environment": "production",
"team": "platform",
"version": "v1.2.3"
},
"additionalProperties": {
- "type": "string"
+ "type": "string",
+ "maxLength": 256
},
"title": "Labels and metadata for the authorization model (similar to Kubernetes labels)"
}
📝 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.
"metadata": { | |
"type": "object", | |
"example": { | |
"environment": "production", | |
"team": "platform", | |
"version": "v1.2.3" | |
}, | |
"additionalProperties": { | |
"type": "string" | |
}, | |
"title": "Labels and metadata for the authorization model (similar to Kubernetes labels)" | |
} | |
"metadata": { | |
"type": "object", | |
"maxProperties": 20, | |
"propertyNames": { | |
"type": "string", | |
"pattern": "^[a-z0-9]([-a-z0-9\\.]{0,61}[a-z0-9])?$", | |
"minLength": 1, | |
"maxLength": 63 | |
}, | |
"example": { | |
"environment": "production", | |
"team": "platform", | |
"version": "v1.2.3" | |
}, | |
"additionalProperties": { | |
"type": "string", | |
"maxLength": 256 | |
}, | |
"title": "Labels and metadata for the authorization model (similar to Kubernetes labels)" | |
} |
🤖 Prompt for AI Agents
In docs/openapiv2/apidocs.swagger.json around lines 720 to 731, the metadata
object lacks validation constraints that match the protobuf layer, such as
limiting to 20 key-value pairs, enforcing DNS-1123 key pattern (non-empty, ≤63
bytes), and value length ≤256 bytes. Update the OpenAPI schema to include these
constraints by adding maxProperties: 20, pattern for keys matching DNS-1123, and
maxLength: 256 for values under additionalProperties, so client SDKs can perform
early validation.
"metadata": { | ||
"type": "object", | ||
"example": { | ||
"environment": "production", | ||
"team": "platform", | ||
"version": "v1.2.3" | ||
}, | ||
"additionalProperties": { | ||
"type": "string" | ||
}, | ||
"title": "Labels and metadata for the authorization model (similar to Kubernetes labels)" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Replicate metadata constraints on the AuthorizationModel response object
Same comment as above – add maxProperties
, propertyNames
, and value length limits so generated clients (e.g., Typescript, Java) carry the constraints.
"metadata": {
"type": "object",
+ "maxProperties": 20,
+ "propertyNames": {
+ "type": "string",
+ "pattern": "^[a-z0-9]([-a-z0-9\\.]{0,61}[a-z0-9])?$",
+ "minLength": 1,
+ "maxLength": 63
+ },
"example": {
"environment": "production",
"team": "platform",
"version": "v1.2.3"
},
"additionalProperties": {
- "type": "string"
+ "type": "string",
+ "maxLength": 256
},
"title": "Labels and metadata for the authorization model (similar to Kubernetes labels)"
}
📝 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.
"metadata": { | |
"type": "object", | |
"example": { | |
"environment": "production", | |
"team": "platform", | |
"version": "v1.2.3" | |
}, | |
"additionalProperties": { | |
"type": "string" | |
}, | |
"title": "Labels and metadata for the authorization model (similar to Kubernetes labels)" | |
} | |
"metadata": { | |
"type": "object", | |
"maxProperties": 20, | |
"propertyNames": { | |
"type": "string", | |
"pattern": "^[a-z0-9]([-a-z0-9\\.]{0,61}[a-z0-9])?$", | |
"minLength": 1, | |
"maxLength": 63 | |
}, | |
"example": { | |
"environment": "production", | |
"team": "platform", | |
"version": "v1.2.3" | |
}, | |
"additionalProperties": { | |
"type": "string", | |
"maxLength": 256 | |
}, | |
"title": "Labels and metadata for the authorization model (similar to Kubernetes labels)" | |
} |
🤖 Prompt for AI Agents
In docs/openapiv2/apidocs.swagger.json around lines 1982 to 1993, the metadata
object lacks constraints such as maxProperties, propertyNames pattern, and value
length limits. Add maxProperties to limit the number of keys, define
propertyNames with a regex pattern to restrict allowed key names, and set
minLength and maxLength on additionalProperties to constrain string value
lengths. This ensures generated clients enforce these constraints properly.
(validate.rules).map.keys.string = { | ||
pattern: "^[a-z0-9]([a-z0-9\\-\\.]*[a-z0-9])?$" | ||
max_bytes: 63 | ||
ignore_empty: true | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore_empty: true
makes empty metadata keys valid – violates the RFC
ignore_empty: true
bypasses the regex when the key is the empty string, yet the spec explicitly says “keys cannot be empty”.
Flip the flag to false
(or omit it – default is false
) to enforce the requirement.
- (validate.rules).map.keys.string = {
- pattern: "^[a-z0-9]([a-z0-9\\-\\.]*[a-z0-9])?$"
- max_bytes: 63
- ignore_empty: true
- },
+ (validate.rules).map.keys.string = {
+ pattern: "^[a-z0-9]([a-z0-9\\-\\.]*[a-z0-9])?$"
+ max_bytes: 63
+ ignore_empty: false
+ },
📝 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.
(validate.rules).map.keys.string = { | |
pattern: "^[a-z0-9]([a-z0-9\\-\\.]*[a-z0-9])?$" | |
max_bytes: 63 | |
ignore_empty: true | |
}, | |
(validate.rules).map.keys.string = { | |
pattern: "^[a-z0-9]([a-z0-9\\-\\.]*[a-z0-9])?$" | |
max_bytes: 63 | |
ignore_empty: false | |
}, |
🤖 Prompt for AI Agents
In openfga/v1/authzmodel.proto around lines 47 to 51, the validation rule for
map keys includes 'ignore_empty: true', which allows empty keys and violates the
specification that keys cannot be empty. Remove the 'ignore_empty: true' line or
set it to false to ensure empty keys are rejected and the regex pattern is
enforced for all keys.
(validate.rules).map.keys.string = { | ||
pattern: "^[a-z0-9]([a-z0-9\\-\\.]*[a-z0-9])?$" | ||
max_bytes: 63 | ||
ignore_empty: true | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty metadata keys slip through here as well
Same issue as in authzmodel.proto
: ignore_empty: true
permits ""
as a key.
Set it to false
to honour the “non-empty key” rule.
- (validate.rules).map.keys.string = {
- pattern: "^[a-z0-9]([a-z0-9\\-\\.]*[a-z0-9])?$"
- max_bytes: 63
- ignore_empty: true
- },
+ (validate.rules).map.keys.string = {
+ pattern: "^[a-z0-9]([a-z0-9\\-\\.]*[a-z0-9])?$"
+ max_bytes: 63
+ ignore_empty: false
+ },
📝 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.
(validate.rules).map.keys.string = { | |
pattern: "^[a-z0-9]([a-z0-9\\-\\.]*[a-z0-9])?$" | |
max_bytes: 63 | |
ignore_empty: true | |
}, | |
(validate.rules).map.keys.string = { | |
pattern: "^[a-z0-9]([a-z0-9\\-\\.]*[a-z0-9])?$" | |
max_bytes: 63 | |
ignore_empty: false | |
}, |
🤖 Prompt for AI Agents
In openfga/v1/openfga_service.proto around lines 1545 to 1549, the validation
rule for map keys currently sets ignore_empty to true, which allows empty string
keys to pass validation. Change ignore_empty to false to enforce the rule that
keys must be non-empty strings.
if len(m.GetMetadata()) > 20 { | ||
err := AuthorizationModelValidationError{ | ||
field: "Metadata", | ||
reason: "value must contain no more than 20 pair(s)", | ||
} | ||
if !all { | ||
return err | ||
} | ||
errors = append(errors, err) | ||
} | ||
|
||
{ | ||
sorted_keys := make([]string, len(m.GetMetadata())) | ||
i := 0 | ||
for key := range m.GetMetadata() { | ||
sorted_keys[i] = key | ||
i++ | ||
} | ||
sort.Slice(sorted_keys, func(i, j int) bool { return sorted_keys[i] < sorted_keys[j] }) | ||
for _, key := range sorted_keys { | ||
val := m.GetMetadata()[key] | ||
_ = val | ||
|
||
if key != "" { | ||
|
||
if len(key) > 63 { | ||
err := AuthorizationModelValidationError{ | ||
field: fmt.Sprintf("Metadata[%v]", key), | ||
reason: "value length must be at most 63 bytes", | ||
} | ||
if !all { | ||
return err | ||
} | ||
errors = append(errors, err) | ||
} | ||
|
||
if !_AuthorizationModel_Metadata_Pattern.MatchString(key) { | ||
err := AuthorizationModelValidationError{ | ||
field: fmt.Sprintf("Metadata[%v]", key), | ||
reason: "value does not match regex pattern \"^[a-z0-9]([a-z0-9\\\\-\\\\.]*[a-z0-9])?$\"", | ||
} | ||
if !all { | ||
return err | ||
} | ||
errors = append(errors, err) | ||
} | ||
|
||
} | ||
|
||
if val != "" { | ||
|
||
if len(val) > 256 { | ||
err := AuthorizationModelValidationError{ | ||
field: fmt.Sprintf("Metadata[%v]", key), | ||
reason: "value length must be at most 256 bytes", | ||
} | ||
if !all { | ||
return err | ||
} | ||
errors = append(errors, err) | ||
} | ||
|
||
} | ||
|
||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Verify empty key validation logic.
The validation logic allows empty keys to bypass validation checks (line 205: if key != ""
). According to the PR objectives, "Keys cannot be empty, while values may be empty." This appears to contradict the requirement.
Please verify that the protobuf definition correctly specifies that keys cannot be empty. The current generated validation code would allow empty keys to pass validation.
🏁 Script executed:
#!/bin/bash
# Description: Check the protobuf definition for metadata field constraints
# Expected: Find validation rules that prevent empty keys
# Search for the metadata field definition in proto files
fd -e proto -x grep -l "metadata" {} \; | head -5 | xargs grep -A 10 -B 2 "metadata.*map"
# Look for validation annotations related to empty keys
rg "validate.*key.*min_len|min_len.*key" --type proto
Length of output: 178
🏁 Script executed:
#!/bin/bash
# Locate the .proto file defining AuthorizationModel
proto_file=$(fd -e proto -x grep -l "message AuthorizationModel" {} | head -1)
echo "Inspecting: $proto_file"
# Show the metadata map field definition (with surrounding context)
grep -Hn -A3 -B2 "metadata" "$proto_file"
# Look for any pgv validation annotations on the metadata map
grep -Hn -C2 "(validate.rules)" "$proto_file"
Length of output: 11391
Enforce non-empty metadata keys
The metadata
map in openfga/v1/authzmodel.proto
is currently annotated with ignore_empty: true
for keys, which allows empty strings to bypass validation. According to our requirements, metadata keys must be non-empty. Please update the protobuf validation rules and regenerate the code:
• File: openfga/v1/authzmodel.proto
(around lines 46–51)
Change the ignore_empty
setting on the key rule:
(validate.rules).map.keys.string = {
pattern: "^[a-z0-9]([a-z0-9\\-\\.]*[a-z0-9])?$"
max_bytes: 63
- ignore_empty: true
+ ignore_empty: false
}
After updating, re-run the code generation so the Go validator enforces non-empty metadata keys.
📝 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.
if len(m.GetMetadata()) > 20 { | |
err := AuthorizationModelValidationError{ | |
field: "Metadata", | |
reason: "value must contain no more than 20 pair(s)", | |
} | |
if !all { | |
return err | |
} | |
errors = append(errors, err) | |
} | |
{ | |
sorted_keys := make([]string, len(m.GetMetadata())) | |
i := 0 | |
for key := range m.GetMetadata() { | |
sorted_keys[i] = key | |
i++ | |
} | |
sort.Slice(sorted_keys, func(i, j int) bool { return sorted_keys[i] < sorted_keys[j] }) | |
for _, key := range sorted_keys { | |
val := m.GetMetadata()[key] | |
_ = val | |
if key != "" { | |
if len(key) > 63 { | |
err := AuthorizationModelValidationError{ | |
field: fmt.Sprintf("Metadata[%v]", key), | |
reason: "value length must be at most 63 bytes", | |
} | |
if !all { | |
return err | |
} | |
errors = append(errors, err) | |
} | |
if !_AuthorizationModel_Metadata_Pattern.MatchString(key) { | |
err := AuthorizationModelValidationError{ | |
field: fmt.Sprintf("Metadata[%v]", key), | |
reason: "value does not match regex pattern \"^[a-z0-9]([a-z0-9\\\\-\\\\.]*[a-z0-9])?$\"", | |
} | |
if !all { | |
return err | |
} | |
errors = append(errors, err) | |
} | |
} | |
if val != "" { | |
if len(val) > 256 { | |
err := AuthorizationModelValidationError{ | |
field: fmt.Sprintf("Metadata[%v]", key), | |
reason: "value length must be at most 256 bytes", | |
} | |
if !all { | |
return err | |
} | |
errors = append(errors, err) | |
} | |
} | |
} | |
} | |
(validate.rules).map.keys.string = { | |
pattern: "^[a-z0-9]([a-z0-9\\-\\.]*[a-z0-9])?$" | |
max_bytes: 63 | |
- ignore_empty: true | |
+ ignore_empty: false | |
} |
🤖 Prompt for AI Agents
In proto/openfga/v1/authzmodel.proto around lines 46 to 51, the metadata map key
validation currently uses ignore_empty: true, allowing empty keys to pass
validation. Remove or set ignore_empty to false for the key validation rule to
enforce non-empty metadata keys. After updating the proto file, regenerate the
Go validation code to ensure the validator properly rejects empty metadata keys.
Summary
This PR adds metadata field support to OpenFGA authorization models, enabling better organization, versioning, and operational management of authorization models.
This PR is required with openfga/openfga#2585
Roadmap
RFC
Community Discussions
Changes
Protocol Buffer Definitions
metadata
field (map<string, string>) with field number 5metadata
field (map<string, string>) with field number 5Validation Rules
^[a-z0-9]([a-z0-9\\-\\.]*[a-z0-9])?$
Generated Code Updates
Features
JSON API Support
Use Cases
environment: production/staging/dev
team: platform/security/backend
version: v1.2.3
compliance: soc2/hipaa
Compatibility
This change is fully backwards compatible:
Database Layer Note
This PR only covers the API definition layer. The actual database schema changes, storage layer implementation, and server-side handling would need to be implemented in the main OpenFGA server repository.
Testing
Summary by CodeRabbit
New Features
Improvements
Bug Fixes