Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Personal Access Token flow: new RPC Changes
sequenceDiagram
participant Client
participant FrontierService
participant TokenStore
participant TokenGenerator
Client->>FrontierService: CreateCurrentUserPersonalToken(request)
FrontierService->>TokenGenerator: generate token value & metadata
TokenGenerator-->>FrontierService: token value
FrontierService->>TokenStore: persist PersonalAccessToken (without token value?) / store metadata & expiry
TokenStore-->>FrontierService: persisted record (id, timestamps)
FrontierService-->>Client: CreateCurrentUserPersonalTokenResponse { token: PersonalAccessToken (includes OUTPUT_ONLY token value) }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
raystack/frontier/v1beta1/frontier.proto (1)
4264-4268:namehas only a length floor; no pattern constraint for a machine-friendly identifier.The inline comment says this is a "Machine-friendly identifier, unique per user per org", yet the only validation is
min_len = 1, which permits arbitrary whitespace, special characters, and single-character names. Every comparablenamefield for machine identifiers in this codebase enforces a pattern (e.g.,^[A-Za-z0-9-_]+$forUser,Group,Organization, etc.).✏️ Suggested constraint
string name = 1 [ - (validate.rules).string.min_len = 1, + (validate.rules).string = { + min_len: 2, + pattern: "^[A-Za-z0-9-_]+$" + }, (google.api.field_behavior) = REQUIRED ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@raystack/frontier/v1beta1/frontier.proto` around lines 4264 - 4268, The `name` field in frontier.proto currently only enforces min_len and should also include a pattern to restrict it to machine-friendly characters; update the `name` field's validation (the same field named "name" in frontier.proto) to add a regex constraint such as `^[A-Za-z0-9-_]+$` (while keeping or adjusting min_len as needed) so it matches other machine identifier fields in the codebase; ensure the `(validate.rules).string.pattern` is added alongside the existing `(validate.rules).string.min_len` and keep the `(google.api.field_behavior) = REQUIRED` intact.raystack/frontier/v1beta1/models.proto (1)
443-444: Consider adding an OpenAPI description and validation hint forname.The
namefield (field 2) has noopenapiv2_fielddescription or validation annotation, unlike comparablenamefields across every other model in this file (e.g.,User.name,Group.name). API consumers won't know what format/constraints to expect.✏️ Suggested addition
- string name = 2; + string name = 2 [(grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { + description: "Machine-friendly identifier for the token. Must be unique per user per organization.", + example: "\"my-ci-token\"" + }];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@raystack/frontier/v1beta1/models.proto` around lines 443 - 444, Add an OpenAPI description and validation hint for the message's `name` field (field 2) similar to other models like `User.name` and `Group.name`: update the `name` field to include an openapiv2_field description explaining expected format (e.g., display name, allowed chars/length) and add proto validation rules (e.g., non-empty, max length, and optional pattern) so API consumers know constraints; target the `name` field (field number 2) and mirror the same description and validate.rules options used by `User.name`/`Group.name`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@raystack/frontier/v1beta1/frontier.proto`:
- Line 4278: The repeated field project_ids currently accepts arbitrary strings;
add the same UUID validation used elsewhere by attaching a validate.rules option
to project_ids (e.g., on the repeated string field declaration for project_ids)
so each item is required to be a UUID; update the field definition for
project_ids to include (validate.rules).string.uuid = true for each element to
ensure boundary validation consistent with org_id and other ID fields.
- Around line 4275-4276: The repeated field roles (repeated string roles = 4) is
annotated as (google.api.field_behavior) = REQUIRED but that does not enforce a
non-empty list at runtime; update the field to add a validator rule by adding
(validate.rules).repeated = { min_items: 1 } to the roles field so the proto
validator enforces at least one role (mirror the user_ids validation pattern
used in CreateOrganizationInvitationRequest).
- Around line 1891-1892: CreateCurrentUserPersonalToken is missing the
grpc-gateway HTTP binding and openapiv2_operation annotations, so add a
google.api.http POST binding and an openapiv2_operation block analogous to other
CurrentUser RPCs (e.g., use POST "/v1beta1/users/me/personal_tokens" with body
"*" or the appropriate request field) and include openapiv2_operation with
operation_id "CreateCurrentUserPersonalToken" plus a short summary/description;
update the RPC declaration for CreateCurrentUserPersonalToken to include these
annotations so it is reachable via REST and appears in the OpenAPI spec.
---
Nitpick comments:
In `@raystack/frontier/v1beta1/frontier.proto`:
- Around line 4264-4268: The `name` field in frontier.proto currently only
enforces min_len and should also include a pattern to restrict it to
machine-friendly characters; update the `name` field's validation (the same
field named "name" in frontier.proto) to add a regex constraint such as
`^[A-Za-z0-9-_]+$` (while keeping or adjusting min_len as needed) so it matches
other machine identifier fields in the codebase; ensure the
`(validate.rules).string.pattern` is added alongside the existing
`(validate.rules).string.min_len` and keep the `(google.api.field_behavior) =
REQUIRED` intact.
In `@raystack/frontier/v1beta1/models.proto`:
- Around line 443-444: Add an OpenAPI description and validation hint for the
message's `name` field (field 2) similar to other models like `User.name` and
`Group.name`: update the `name` field to include an openapiv2_field description
explaining expected format (e.g., display name, allowed chars/length) and add
proto validation rules (e.g., non-empty, max length, and optional pattern) so
API consumers know constraints; target the `name` field (field number 2) and
mirror the same description and validate.rules options used by
`User.name`/`Group.name`.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
raystack/frontier/v1beta1/models.proto (1)
442-466: Consider addingreservedfor intentionally skipped field numbers.The message has deliberate gaps — 3-4 (between
titleandtoken), 6-9 (betweentokenandexpires_at), and 13-19 (betweencreated_atandmetadata) — with noreservedstatement to document them. Withoutreserved, future contributors can silently reuse those numbers, causing silent wire-format collisions with any data serialized against later versions of this message.♻️ Proposed addition
message PersonalAccessToken { string id = 1; string title = 2; + + reserved 3, 4; + // token will only be returned once as part of the create process // this value is never persisted in the system so if lost, can't be recovered string token = 5 [(google.api.field_behavior) = OUTPUT_ONLY]; + + reserved 6 to 9; google.protobuf.Timestamp expires_at = 10 [...]; google.protobuf.Timestamp last_used_at = 11 [...]; google.protobuf.Timestamp created_at = 12 [...]; + + reserved 13 to 19; google.protobuf.Struct metadata = 20; }The rest of the message — the
OUTPUT_ONLYannotation ontoken, theexpires_at/last_used_at/created_atgrouping, andmetadata = 20— all follow established file conventions and look correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@raystack/frontier/v1beta1/models.proto` around lines 442 - 466, The PersonalAccessToken message is missing reserved declarations for the intentionally skipped field numbers; add a reserved statement in the PersonalAccessToken message reserving the numeric ranges 3 to 4, 6 to 9, and 13 to 19 (e.g., reserved 3 to 4, 6 to 9, 13 to 19;) so future changes cannot reuse those tags and cause wire-format collisions; place this reserved line near the top of the message alongside the field declarations for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@raystack/frontier/v1beta1/models.proto`:
- Around line 442-466: The PersonalAccessToken message is missing reserved
declarations for the intentionally skipped field numbers; add a reserved
statement in the PersonalAccessToken message reserving the numeric ranges 3 to
4, 6 to 9, and 13 to 19 (e.g., reserved 3 to 4, 6 to 9, 13 to 19;) so future
changes cannot reuse those tags and cause wire-format collisions; place this
reserved line near the top of the message alongside the field declarations for
clarity.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
raystack/frontier/v1beta1/models.proto (2)
452-470: Server-generated timestamp fields are missingOUTPUT_ONLYfield behavior.
token(field 5) is correctly annotated with(google.api.field_behavior) = OUTPUT_ONLY, but the four server-assigned timestamp fields are not. This inconsistency can mislead clients into believing these fields are settable.♻️ Proposed fix
- google.protobuf.Timestamp expires_at = 10 [(grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { + google.protobuf.Timestamp expires_at = 10 [ + (google.api.field_behavior) = OUTPUT_ONLY, + (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { description: "The time when the token expires.", example: "\"2024-06-07T05:39:56.961Z\"" - }]; + }]; - google.protobuf.Timestamp last_used_at = 11 [(grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { + google.protobuf.Timestamp last_used_at = 11 [ + (google.api.field_behavior) = OUTPUT_ONLY, + (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { description: "The time when the token was last used.", example: "\"2024-06-07T05:39:56.961Z\"" - }]; + }]; - google.protobuf.Timestamp created_at = 12 [(grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { + google.protobuf.Timestamp created_at = 12 [ + (google.api.field_behavior) = OUTPUT_ONLY, + (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { description: "The time when the token was created.", example: "\"2023-06-07T05:39:56.961Z\"" - }]; + }]; - google.protobuf.Timestamp updated_at = 13 [(grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { + google.protobuf.Timestamp updated_at = 13 [ + (google.api.field_behavior) = OUTPUT_ONLY, + (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { description: "The time when the token was last updated.", example: "\"2023-06-07T05:39:56.961Z\"" - }]; + }];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@raystack/frontier/v1beta1/models.proto` around lines 452 - 470, The four server-assigned timestamp fields expires_at, last_used_at, created_at, and updated_at should be marked as output-only to match token (field 5); update each field's options to include (google.api.field_behavior) = OUTPUT_ONLY so clients know these are server-generated and not settable (edit the definitions of expires_at, last_used_at, created_at, updated_at in the same message in models.proto to add the google.api.field_behavior annotation).
442-473: Reserve the intentionally-skipped field number ranges.Field numbers jump 5 → 10 and 13 → 20 with no
reserveddeclarations. Without them, a future editor can accidentally reuse one of the skipped numbers, silently producing wire-format collisions.♻️ Proposed fix: add reserved declarations
message PersonalAccessToken { string id = 1; string title = 2; string user_id = 3; string org_id = 4; // token will only be returned once as part of the create process // this value is never persisted in the system so if lost, can't be recovered string token = 5 [(google.api.field_behavior) = OUTPUT_ONLY]; + reserved 6 to 9; google.protobuf.Timestamp expires_at = 10 [...] google.protobuf.Timestamp last_used_at = 11 [...] google.protobuf.Timestamp created_at = 12 [...] google.protobuf.Timestamp updated_at = 13 [...] + reserved 14 to 19; google.protobuf.Struct metadata = 20; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@raystack/frontier/v1beta1/models.proto` around lines 442 - 473, Add explicit reserved ranges in the PersonalAccessToken message to prevent accidental reuse of the intentionally-skipped field numbers: declare reserved 5 to 9 and reserved 14 to 19 (e.g., using reserved 5 to 9; reserved 14 to 19;) inside the PersonalAccessToken message definition (near the other field declarations) so future edits cannot reuse those numeric tags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@raystack/frontier/v1beta1/models.proto`:
- Around line 452-470: The four server-assigned timestamp fields expires_at,
last_used_at, created_at, and updated_at should be marked as output-only to
match token (field 5); update each field's options to include
(google.api.field_behavior) = OUTPUT_ONLY so clients know these are
server-generated and not settable (edit the definitions of expires_at,
last_used_at, created_at, updated_at in the same message in models.proto to add
the google.api.field_behavior annotation).
- Around line 442-473: Add explicit reserved ranges in the PersonalAccessToken
message to prevent accidental reuse of the intentionally-skipped field numbers:
declare reserved 5 to 9 and reserved 14 to 19 (e.g., using reserved 5 to 9;
reserved 14 to 19;) inside the PersonalAccessToken message definition (near the
other field declarations) so future edits cannot reuse those numeric tags.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
raystack/frontier/v1beta1/models.proto (1)
442-482: Considerreserveddeclarations for intentional field-number gapsFields jump from 5 → 10 and from 13 → 20, leaving field numbers 6–9 and 14–19 unused with no
reservedclause. Theprotoccompiler will generate error messages if any future developer tries to use a reserved field number, so addingreservedranges here turns an unspoken convention into an enforced invariant.♻️ Proposed refactor
string token = 5 [(google.api.field_behavior) = OUTPUT_ONLY]; + reserved 6 to 9; + google.protobuf.Timestamp expires_at = 10 [ ... google.protobuf.Timestamp updated_at = 13 [ ... ]; + reserved 14 to 19; + google.protobuf.Struct metadata = 20;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@raystack/frontier/v1beta1/models.proto` around lines 442 - 482, The PersonalAccessToken message has unused field numbers (6–9 and 14–19); add reserved declarations inside message PersonalAccessToken to explicitly reserve those numeric ranges (e.g., reserved 6 to 9; reserved 14 to 19;) so future developers cannot accidentally reuse those tags and protoc will enforce the invariant; place the reserved lines near the top of the message definition after existing field declarations for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@raystack/frontier/v1beta1/models.proto`:
- Around line 452-455: The expires_at field is missing the OUTPUT_ONLY field
behavior: update the expires_at definition to include the
google.api.field_behavior OUTPUT_ONLY option (matching last_used_at, created_at,
updated_at) so the proto signals the field is server-set; e.g., add
[(google.api.field_behavior) = OUTPUT_ONLY] to the expires_at field annotation
and ensure the file imports google/api/field_behavior.proto if not already
present.
- Line 445: The user_id field in the message should be marked OUTPUT_ONLY so
clients don't treat it as writable; update the field declaration for user_id
(string user_id = 3) to include the protobuf field behavior annotation
(google.api.field_behavior) = OUTPUT_ONLY (i.e., add
[(google.api.field_behavior) = OUTPUT_ONLY]) so code generators and client
libraries know it is server-derived; ensure the google/api/annotations.proto
import is present if not already and apply this change to the message that
defines user_id (e.g., the CreateCurrentUserPersonalTokenRequest/related
response message) so the field is treated as output-only.
---
Nitpick comments:
In `@raystack/frontier/v1beta1/models.proto`:
- Around line 442-482: The PersonalAccessToken message has unused field numbers
(6–9 and 14–19); add reserved declarations inside message PersonalAccessToken to
explicitly reserve those numeric ranges (e.g., reserved 6 to 9; reserved 14 to
19;) so future developers cannot accidentally reuse those tags and protoc will
enforce the invariant; place the reserved lines near the top of the message
definition after existing field declarations for clarity.
| message PersonalAccessToken { | ||
| string id = 1; | ||
| string title = 2; | ||
| string user_id = 3; |
There was a problem hiding this comment.
user_id should carry OUTPUT_ONLY
user_id is not present in CreateCurrentUserPersonalTokenRequest — it is derived from the authenticated session server-side. Without OUTPUT_ONLY, code generators and client libraries may incorrectly treat it as a writable field.
🛡️ Proposed fix
- string user_id = 3;
+ string user_id = 3 [(google.api.field_behavior) = OUTPUT_ONLY];📝 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.
| string user_id = 3; | |
| string user_id = 3 [(google.api.field_behavior) = OUTPUT_ONLY]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@raystack/frontier/v1beta1/models.proto` at line 445, The user_id field in the
message should be marked OUTPUT_ONLY so clients don't treat it as writable;
update the field declaration for user_id (string user_id = 3) to include the
protobuf field behavior annotation (google.api.field_behavior) = OUTPUT_ONLY
(i.e., add [(google.api.field_behavior) = OUTPUT_ONLY]) so code generators and
client libraries know it is server-derived; ensure the
google/api/annotations.proto import is present if not already and apply this
change to the message that defines user_id (e.g., the
CreateCurrentUserPersonalTokenRequest/related response message) so the field is
treated as output-only.
| google.protobuf.Timestamp expires_at = 10 [(grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { | ||
| description: "The time when the token expires.", | ||
| example: "\"2024-06-07T05:39:56.961Z\"" | ||
| }]; |
There was a problem hiding this comment.
expires_at is inconsistently missing OUTPUT_ONLY
last_used_at (field 11), created_at (field 12), and updated_at (field 13) all carry OUTPUT_ONLY, but expires_at (field 10) does not. Even though expires_at is accepted as input in the request, the value that is actually persisted and returned is server-authoritative — it may be clamped or normalized. The response-model field should be marked OUTPUT_ONLY for consistency and to correctly signal to generated clients that the echoed value is server-set.
🛡️ Proposed fix
- google.protobuf.Timestamp expires_at = 10 [(grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = {
- description: "The time when the token expires.",
- example: "\"2024-06-07T05:39:56.961Z\""
- }];
+ google.protobuf.Timestamp expires_at = 10 [
+ (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = {
+ description: "The time when the token expires.",
+ example: "\"2024-06-07T05:39:56.961Z\""
+ },
+ (google.api.field_behavior) = OUTPUT_ONLY
+ ];📝 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.
| google.protobuf.Timestamp expires_at = 10 [(grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { | |
| description: "The time when the token expires.", | |
| example: "\"2024-06-07T05:39:56.961Z\"" | |
| }]; | |
| google.protobuf.Timestamp expires_at = 10 [ | |
| (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { | |
| description: "The time when the token expires.", | |
| example: "\"2024-06-07T05:39:56.961Z\"" | |
| }, | |
| (google.api.field_behavior) = OUTPUT_ONLY | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@raystack/frontier/v1beta1/models.proto` around lines 452 - 455, The
expires_at field is missing the OUTPUT_ONLY field behavior: update the
expires_at definition to include the google.api.field_behavior OUTPUT_ONLY
option (matching last_used_at, created_at, updated_at) so the proto signals the
field is server-set; e.g., add [(google.api.field_behavior) = OUTPUT_ONLY] to
the expires_at field annotation and ensure the file imports
google/api/field_behavior.proto if not already present.
…dd UUID validation
1 participant
Summary by CodeRabbit