feat(cli): add check, schema, data, and tenant management commands#2835
Open
HarshadaGawas05 wants to merge 2 commits intoPermify:masterfrom
Open
feat(cli): add check, schema, data, and tenant management commands#2835HarshadaGawas05 wants to merge 2 commits intoPermify:masterfrom
HarshadaGawas05 wants to merge 2 commits intoPermify:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds four gRPC-based CLI commands (check, schema, data, tenant) plus supporting credential loading, gRPC dialing/TLS, reference parsing, output formatting, context timeout helpers, and test stubs; registers the new commands in the root CLI and updates .gitignore and Makefile. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Check Command
participant Creds as Credentials Module
participant GRPC as gRPC Dialer
participant Server as Permify Server
User->>CLI: run `permify check --tenant-id ... --entity ... --resource ... --permission ...`
CLI->>CLI: validate flags, parse refs
CLI->>Creds: Resolve & load credentials
Creds-->>CLI: CredentialsFile (endpoint, token, tls_ca)
CLI->>GRPC: DialGRPC(endpoint, options)
GRPC-->>CLI: grpc.ClientConn
CLI->>Server: PermissionClient.Check(request)
Server-->>CLI: PermissionCheckResponse (can: true/false, metadata)
CLI->>CLI: formatCheckResult(writer, response)
CLI->>User: print allowed/denied and metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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)
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 Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
pkg/cmd/parse_refs_test.go (1)
18-30: Consider table-driven invalid-case coverage for parser boundaries.Current tests miss cases like empty input,
:id,type:, andtype:id:extra. Adding these would lock parser behavior and prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/parse_refs_test.go` around lines 18 - 30, Add a table-driven test covering parser boundary cases for ParseEntityRef (and similarly ParseSubjectRef if applicable): replace or augment TestParseEntityRef_Invalid with a slice of invalid inputs such as "", ":id", "type:", "type:id:extra", "nocolon" and iterate asserting ParseEntityRef returns an error for each input; also add a parallel table-driven test for ParseSubjectRef to assert errors on invalid subject inputs (e.g., "", ":id", "type:", "type:id:extra") while keeping calls to ParseSubjectRef/ParseEntityRef and using require.Error/require.NoError as appropriate to lock down behavior.pkg/cmd/tenant.go (1)
75-77: Redundant default assignment forpageSize.The flag definition on line 95 already sets the default to
100, so the check on lines 75-77 is unnecessary—pageSizewill never be0unless explicitly set by the user.♻️ Suggested simplification
cmd := &cobra.Command{ Use: "list", Short: "List tenants (Tenancy.List)", RunE: func(cmd *cobra.Command, _ []string) error { - if pageSize == 0 { - pageSize = 100 - } - conn, err := DialGRPC(credentialsPath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/tenant.go` around lines 75 - 77, The conditional block that assigns pageSize = 100 when pageSize == 0 is redundant because the flag definition already sets the default to 100; remove the if block that checks pageSize (the snippet containing "if pageSize == 0 { pageSize = 100 }") and rely on the flag's default; if you want to validate user input instead, replace the removal with an explicit validation that returns an error when pageSize <= 0, referencing the pageSize variable and the flag definition that sets its default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Line 32: The .gitignore entry "permify" is unanchored and may match any path
segment named permify; update the rule in .gitignore to anchor the generated
root binary by replacing the unanchored token with an anchored entry (prefix
with a slash) so the ignore specifically targets ./permify.
In `@pkg/cmd/credentials.go`:
- Around line 61-63: Trimmed c.TLSCAPath is currently treated as relative to the
process CWD; change the loader so that after lines trimming c.Endpoint,
c.APIToken, c.TLSCAPath you resolve c.TLSCAPath against the directory containing
the credentials file: if c.TLSCAPath is non-empty and not an absolute path, join
it with filepath.Dir(<the credentials file path variable used to load the
credentials>) and assign the result back to c.TLSCAPath so subsequent CA loading
uses the correct absolute path.
In `@pkg/cmd/data.go`:
- Around line 166-168: The code currently only sets a default when pageSize == 0
but does not enforce the documented 1–100 range; update the validation for the
pageSize variable in pkg/cmd/data.go (the code that reads the --page-size flag)
to ensure it falls within 1..100 — e.g., if pageSize < 1 or pageSize > 100
return a user-facing error (or clamp to 100 if you prefer) so values above 100
are rejected/adjusted to match the documentation.
- Around line 118-127: Validation currently allows whitespace-only tuple fields
and reports zero-based tuple indices; update the loop that iterates doc.Tuples
to trim whitespace from row.Entity.Type, row.Entity.ID, row.Subject.Type,
row.Subject.ID and row.Relation (use strings.TrimSpace) before validating, and
change the error messages in those checks to report the tuple number as i+1
instead of i; ensure you reference the same identifiers (doc.Tuples,
row.Entity.Type, row.Entity.ID, row.Subject.Type, row.Subject.ID, row.Relation)
so callers see trimmed values and 1-based indices in errors.
In `@pkg/cmd/parse_refs.go`:
- Around line 30-38: splitTypeID currently allows multiple colons (e.g.
"user:alice:owner") by using Index only; change the validation in splitTypeID to
require exactly one colon (for example use strings.Count(s, ":") != 1 or compare
strings.Index and strings.LastIndex) and return the same "expected type:id (e.g.
user:1)" error when the count is not exactly one, otherwise split on that single
colon and return typ and id.
---
Nitpick comments:
In `@pkg/cmd/parse_refs_test.go`:
- Around line 18-30: Add a table-driven test covering parser boundary cases for
ParseEntityRef (and similarly ParseSubjectRef if applicable): replace or augment
TestParseEntityRef_Invalid with a slice of invalid inputs such as "", ":id",
"type:", "type:id:extra", "nocolon" and iterate asserting ParseEntityRef returns
an error for each input; also add a parallel table-driven test for
ParseSubjectRef to assert errors on invalid subject inputs (e.g., "", ":id",
"type:", "type:id:extra") while keeping calls to ParseSubjectRef/ParseEntityRef
and using require.Error/require.NoError as appropriate to lock down behavior.
In `@pkg/cmd/tenant.go`:
- Around line 75-77: The conditional block that assigns pageSize = 100 when
pageSize == 0 is redundant because the flag definition already sets the default
to 100; remove the if block that checks pageSize (the snippet containing "if
pageSize == 0 { pageSize = 100 }") and rely on the flag's default; if you want
to validate user input instead, replace the removal with an explicit validation
that returns an error when pageSize <= 0, referencing the pageSize variable and
the flag definition that sets its default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e72409b-f673-4b8a-9c4e-ec40785d1c1a
📒 Files selected for processing (19)
.gitignoreMakefilecmd/permify/permify.gopkg/cmd/check.gopkg/cmd/check_test.gopkg/cmd/credentials.gopkg/cmd/credentials_test.gopkg/cmd/data.gopkg/cmd/data_test.gopkg/cmd/grpc_context.gopkg/cmd/grpc_mocks_test.gopkg/cmd/grpc_output.gopkg/cmd/grpc_output_test.gopkg/cmd/parse_refs.gopkg/cmd/parse_refs_test.gopkg/cmd/schema.gopkg/cmd/schema_test.gopkg/cmd/tenant.gopkg/cmd/tenant_test.go
Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/cmd/credentials.go (1)
89-89: Minor: redundantTrimSpacecall.
c.APITokenis already trimmed inLoadCredentialsat line 62. This duplicate trim is harmless but unnecessary.♻️ Suggested simplification
func GRPCDialOptions(c *CredentialsFile) ([]grpc.DialOption, error) { - token := strings.TrimSpace(c.APIToken) + token := c.APIToken if token == "" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/credentials.go` at line 89, Remove the redundant trimming when assigning token in credentials.go: since LoadCredentials already trims and sets c.APIToken, replace or simplify the assignment token := strings.TrimSpace(c.APIToken) to just token := c.APIToken (or otherwise use c.APIToken directly) in the function where token is created; keep references to c.APIToken, token, and LoadCredentials to ensure callers rely on the already-trimmed value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/cmd/credentials.go`:
- Line 89: Remove the redundant trimming when assigning token in credentials.go:
since LoadCredentials already trims and sets c.APIToken, replace or simplify the
assignment token := strings.TrimSpace(c.APIToken) to just token := c.APIToken
(or otherwise use c.APIToken directly) in the function where token is created;
keep references to c.APIToken, token, and LoadCredentials to ensure callers rely
on the already-trimmed value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d1be37c-a327-4ceb-85b0-b08f49fbfa51
📒 Files selected for processing (6)
.gitignorepkg/cmd/credentials.gopkg/cmd/credentials_test.gopkg/cmd/data.gopkg/cmd/parse_refs.gopkg/cmd/parse_refs_test.go
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- pkg/cmd/credentials_test.go
- pkg/cmd/data.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/cmd/parse_refs_test.go
- pkg/cmd/parse_refs.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
Implements complete CLI management commands for interacting with a running Permify instance via gRPC, addressing #146.
Adds four command groups that were completely missing from the CLI:
permify check— evaluate permissions via Permission.Check RPCpermify schema write|read— manage authorization schemaspermify data write|read— write/read relationship tuplespermify tenant create|list|delete— manage tenantsCredentials are loaded from
~/.permify/credentials(YAML). Whenapi_tokenis set, TLS is used automatically (system trust store,or custom CA via
tls_ca_path). Without a token, insecure transportis used for local development.
Improvements over existing PR #2799:
TLS instead of insecure transport
configure)Proof
New Files
pkg/cmd/credentials.gopkg/cmd/check.gopermify checkcommandpkg/cmd/schema.gopermify schema write|readcommandspkg/cmd/data.gopermify data write|readcommandspkg/cmd/tenant.gopermify tenant create|list|deletecommandspkg/cmd/parse_refs.gotype:idstrings into protobuf typespkg/cmd/grpc_output.gopkg/cmd/grpc_context.gopkg/cmd/*_test.goChecklist
/claim #146
Summary by CodeRabbit