Skip to content

Conversation

@matt-codecov
Copy link
Contributor

@matt-codecov matt-codecov commented Dec 12, 2025

Signing tokens in the client requires structured scope information which we currently don't have. We have a structured scope type in objectstore_service already so this PR moves that type to objectstore_types and integrates it into the Rust client.

As a driveby, it also moves the Permission type in preparation for the actual token signing impl.

@matt-codecov matt-codecov requested a review from a team as a code owner December 12, 2025 01:59
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jan-auer
Copy link
Member

Responding to comments from the original PR description, these should all be sufficiently addressed.

objectstore_service: AsStoragePath should stay put, but that means Scopes::as_storage_path() has to go away

Those are now two separate types. The only reason it was a single type is because they were in the same module and we didn't have to create a second struct. Those are simple view wrappers (a common Rust pattern), so there's no need to have them as one type.

client: naming. the rust client already had a Scope type but it isn't a 1:1 analogue because the pre-existing type is how the usecase is plumbed into sessions. we should refactor or rename it. this is probably not a huge change, i just ran out of steam

The client API is rather intentional here and suggests a specific semantic data model. Maybe we should rename Scope to Scopes, but otherwise we can keep it exactly as-is. It's also good that our internal Scopes type is wrapped, as this allows us to introduce variations between how we use it in the server (fast-moving changes) and the client (stable, slow moving API).

client: lots of allocations when formatting the structured scope information

  • allocate a string for each key and each value when building the Scopes type

This is a limitation of structured scopes we can accept. We could also write them directly into a string and then parse it on-demand when we build the token, but I'm not sure this is worth the complexity. If we truly cause a performance issue through these allocations, we can still optimize without breaking the API.

  • allocate a {key}={value} string for each key/value pair when formatting to path segment
  • allocate a Vec<String> for all the key/value pairs in the path segment
  • allocate a String to join the vector elements with ;s

Fixed by using a view wrapper .

  • clone each key and each value when transforming into a BTreeMap for JWT serialization

Not part of this PR.

  • client: breaking API change, the for_project() and for_organization() functions now return Result<Scope> instead of Scope

Reverted. The Scope type already had an internal Result to intentionally defer when the error is exposed so that users can simply chain multiple scopes and then handle the error at a single place. We now use this internal Result as intended.

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would appreciate another review from @lcian since I made substantial changes to the PR.

Copy link
Member

@lcian lcian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good!

@jan-auer jan-auer merged commit 0ad3d0e into main Dec 12, 2025
19 checks passed
@jan-auer jan-auer deleted the matt/moving-types branch December 12, 2025 10:21
matt-codecov added a commit that referenced this pull request Dec 16, 2025
mint tokens in our client library if a secret key and other config has
been passed in

i will update #231 for
end-to-end testing of both clients

depends on #236

Ref FS-202
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants