feat(sdk): DSPX-2418 add attribute discovery methods#841
Conversation
Summary of ChangesHello @marythought, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the SDK's capabilities for interacting with the platform's attribute system. It introduces a suite of new functions designed to simplify the process of discovering available attributes, validating attribute FQNs against the platform, and retrieving attributes associated with specific entities. These additions aim to improve developer experience by providing robust tools for attribute management and early detection of configuration issues, thereby streamlining the development of applications that rely on attribute-based access control. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces several useful convenience methods for attribute discovery and validation, along with a new AttributeNotFoundError. While the implementation is generally well-structured, there are critical security concerns regarding the handling of user-supplied URLs and FQNs, which could lead to reflected Cross-Site Scripting (XSS) and potential credential theft if not properly addressed. Additionally, I've provided a couple of suggestions to improve code clarity and conciseness in the new discovery.ts file.
Add four convenience methods for attribute discovery and validation: - listAttributes(platformUrl, authProvider, namespace?) — auto-paginates through all active attributes; optional namespace filter - validateAttributes(platformUrl, authProvider, fqns[]) — validates FQN format then confirms existence on the platform; throws AttributeNotFoundError for missing FQNs; capped at 250 FQNs to match the server limit - validateAttributeValue(platformUrl, authProvider, fqn) — single-FQN convenience wrapper around validateAttributes - getEntityAttributes(platformUrl, authProvider, entity) — returns the attribute value FQNs assigned to a PE or NPE via GetEntitlements Also adds AttributeNotFoundError (extends TdfError) exported from the package root for callers to distinguish "not found" from other errors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Mary Dickson <mary.dickson@virtru.com>
c9abaa5 to
08b8f54
Compare
X-Test Failure Report |
…dd new validateAttributeValue The old validateAttributeValue(fqn) only checked if a complete value FQN was pre-registered on the platform. This gave false negatives for dynamic (non-enumerated) attributes where any string value is valid. - Rename validateAttributeValue(fqn) → validateAttributeExists(fqn). - Add validateAttributeValue(platformUrl, authProvider, attributeFqn, value) which fetches the attribute definition via getAttribute: - Enumerated attribute (values non-empty): value must match case-insensitively. - Dynamic attribute (values empty): any string is accepted. - Add ATTRIBUTE_FQN_RE regex for attribute-level FQN validation. - Export validateAttributeExists from index.ts. - Add 2 new input-validation tests; rename existing validateAttributeValue describe block to validateAttributeExists. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
If these changes look good, signoff on them with: If they aren't any good, please remove them with: |
Signed-off-by: Mary Dickson <mary.dickson@virtru.com>
73e7966 to
580b0f8
Compare
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove getEntityAttributes: GetEntitlements is server-side/PEP only; in browser contexts there is no way to distinguish PEP auth from user auth, risking entitlement mining of other users - Set explicit LIST_ATTRIBUTES_PAGE_SIZE=1000 (matches platform default) so listAttributes behavior is stable regardless of server config changes - Reduce MAX_LIST_ATTRIBUTES_PAGES from 1000 to 10 (10k record cap is generous for browser use) - Improve page-limit error message to explain the cause and suggest the namespace filter - Clarify MAX_VALIDATE_FQNS=250 comment: traces to max_items:250 in the GetAttributeValuesByFqnsRequest proto constraint - Include value and attributeFqn in validateAttributeValue error message Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
If these changes look good, signoff on them with: If they aren't any good, please remove them with: |
Signed-off-by: Mary Dickson <mary.dickson@virtru.com>
fc30fdf to
e8b66cd
Compare
eugenioenko
left a comment
There was a problem hiding this comment.
Code changes look good, the addition of tests also helps.
Nicely done!
|
One thing I'm noticing and it's already an established pattern, when we need isolated functions to call endpoints we end up instantiating a new PlatformClient. Ex: Instantiating a new client each time is more expensive, Connect RPC recommends avoiding creating new transport each time, here some reference: This is not something we need to fix in this PR, but I feel like we need to give it some thought and it might affect some of our interfaces, so that's something to keep in mind |
An attribute with no registered values now throws AttributeNotFoundError instead of silently succeeding. Also aligns error messages with Go and Java SDKs, and includes the FQN in the attribute-not-found error. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…eValue with attributeExists/attributeValueExists Both methods now return bool instead of throwing on not-found, aligning with idiomatic TypeScript conventions for existence checks. ConnectError with Code.NotFound is caught and mapped to false; other errors propagate as NetworkError. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Add five discovery methods in
lib/src/policy/discovery.ts, exported from the package root:listAttributes(),validateAttributes(),attributeExists(),attributeValueExists(),getEntityAttributes()-- REMOVED (not applicable)Add
AttributeNotFoundError extends TdfError, exported from the package root so callers candistinguish "not found" from other errors
Add unit tests in
discovery.spec.tscovering all methods and edge casesdocs update: Feat: DSPX-2418 add SDK convenience/discovery method documentation docs#186
Rationale
Addresses GitHub discussion developer feedback. These methods provide ergonomic wrappers over the
lower-level
PlatformClientthat:listAttributes()with a 1,000-page safety cap to prevent unbounded memorygrowth from a misbehaving server
createZTDF()so failures happen at encryption timewith a clear message rather than at decryption time with a cryptic "resource not found"
GetEntitlementsresponse to prevent silent wrong-entity attributereturns
validateAttributes()at 250 FQNs client-side to match the server-side limitMatches the API shape and security properties of the Go SDK implementation: opentdf/platform#3082.
Test plan
npm testinlib/— all tests pass, no regressions