Helper party api key #37
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes focus on enhancing local development environment configurations and managing API keys for helper parties. This includes updating the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- .gitignore (1 hunks)
- server/.env.development (1 hunks)
- server/data/supabaseTypes.ts (1 hunks)
- server/supabase/migrations/20240609010604_helper_party_api_keys.sql (1 hunks)
- server/supabase/seed.sql (1 hunks)
Files skipped from review due to trivial changes (3)
- .gitignore
- server/.env.development
- server/supabase/seed.sql
Additional comments not posted (4)
server/supabase/migrations/20240609010604_helper_party_api_keys.sql (3)
1-9: The table structure and default values are well-defined and align with the requirements for API key management.
11-11: Enabling row-level security is a crucial step for protecting sensitive data in the table.
13-13: It's important to ensure that the application logic strictly adheres to the security guidelines mentioned in the comment to prevent unauthorized access to API keys.server/data/supabaseTypes.ts (1)
55-89: The type definitions forhelper_party_api_keysare comprehensive and correctly reflect the database schema. The relationships are well-defined, ensuring proper foreign key constraints.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- server/.env.development (1 hunks)
- server/data/supabaseTypes.ts (2 hunks)
- server/supabase/migrations/20240609010604_helper_party_api_keys.sql (1 hunks)
- server/supabase/seed.sql (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- server/.env.development
- server/data/supabaseTypes.ts
- server/supabase/migrations/20240609010604_helper_party_api_keys.sql
- server/supabase/seed.sql
eriktaubeneck
changed the title
| hashed_api_key varchar(255) not null, | ||
| created_at timestamp default current_timestamp not null, | ||
| expires_at timestamp default current_timestamp + interval '1 year' not null, | ||
| revoked boolean default false not null |
There was a problem hiding this comment.
can you elaborate why we need an extra flag here? One may think that to revoke a key, you can set expires_at in the past and wait until this change is propagated
There was a problem hiding this comment.
Main reason is so that we retain more information, e.g., revoking a key is different than it expiring, and we'd want to provide different error messages based on those different facts.
There was a problem hiding this comment.
My main concern is the validity check - you must not forget to check both and if expires_at is still in the future and revoked is set, that looks weird. Maybe, as an alternative, you could use status or modify_reason/modified_at? Validity check can only look at expires_at and that is simpler
There was a problem hiding this comment.
If we have a status column, then it becomes a bit less clear to a future developer that they need to do a validity check on expires_at and not status, and similarly with modify_reason/modify_at you need to assure that it also changes expiry.
Here's another idea. It would require slightly more work, but would be easier to worry about. Create two tables:
helper_party_api_key_creation_events and helper_party_api_key_revocation_events. The first table would have everything in this PR except the revoked column, and the second table would have: uuid, helper_party_api_key_creation_event_uuid (join column), helper_party_uuid, created_at.
Then, we'd create a view (possibly materialized), which would be used for the validity checks
select
c.helper_party_uuid,
, c.hashed_api_key
, c.expires_at < current_timestamp() and r.uuid is null as valid
, case
when r.uuid is not null as 'revoked'
when c.expires_at >= current_timestamp() as 'expired'
else null
from
helper_party_api_key_creation_events c
left outer join
helper_party_api_key_revocation_events r
on c.uuid = r.helper_party_api_key_creation_event_uuid
This does a few nice things:
- Maintains all information (e.g., nothing is overwritten or modified, just computed as a result of events.)
- Minimizes the risk of incorrect validation, as stored directly on the table.
- Minimized the risk of incorrectly invalidating, as a future developer just needs to create that event, rather than make sure to properly modify the
helper_party_api_keytable.
The only potential downside is that it moves some of business logic into the database. For this, it seems like there are essentially 3 places where logic can live (and things can go wrong):
- When events are created or modified (revoked). In your example, this would require making sure that when a revoke action happens, the developer needs to correctly update the expiration.
- When api keys are validated. This requires potentially checking expires_at and revoked.
- In the database, as a view computed over actions.
After writing this all out, I think I like 3/ the best. Also, this type of event driven architecture is a possible solution to the problem you mention about state management across between the management plane and multiple control planes.
There was a problem hiding this comment.
With the event sourcing, you typically have only one timestamp which is the timestamp of the event. Then you can build a snapshot and/or the latest status by applying these events according to their timestamp and looking at the resulting object. So you technically can have only one table that records two event types - create/revoke and if revoke is the last event, then key is no longer valid.
This could be more powerful than what we want here - I was thinking that expiry_at + modify_reason = "REVOKE" is enough information for an engineer to see what is going on, even if reason is unstructured. I don't want to overcompicate things here, so if you are not inclined with this, I think the current proposal is better than even sourcing because it is less complex
| @@ -0,0 +1,13 @@ | |||
| create table | |||
| helper_party_api_keys ( | |||
There was a problem hiding this comment.
Most of the time I see API keys bound to clients that use them. Helper party has the power to revoke them. but they also want to know who requested them and why, so they can validate both at the request time. If you allow more than one key per party, that may be worth exploring, because otherwise it gets hard to manage
There was a problem hiding this comment.
This isn't super clear from the PR, but it will be more clear in the next one: the point of these keys for the report collector (draft/server) to provide a key to the helper party (draft/sidecar), that allows that helper party to POST a status update about a query to the server. This is issued from the report collector to the helper party, so that the helper party can validate that POSTs coming in from helper parties are valid.
There was a problem hiding this comment.
I see. So, generally systems that are allowed to talk to each other using bi-directional channels, that is: sending actions/mutating the state on both sides are much harder to reason about. Both sides need to have "write" permission on the other side which leads to security complications and maintainability issues.
For the short term, it can work but I'd love us to reconsider this approach after the test
There was a problem hiding this comment.
yea, a more detailed design review here would be great. at a very high level, I'm not sure we can avoid bi-directional all together, but I certainly want to (and think we can) avoid managing state in both places. As I see it:
- Management Plane --> Control Plane: Start query signal, Stop Query signal, etc
- Control Plane --> Management Plane: Current state, logs, observability
So different signals, but in both directions, we need authentication from between these two planes (as they are operated by different parties.)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- server/data/supabaseTypes.ts (1 hunks)
- server/supabase/migrations/20240609010604_helper_party_api_keys.sql (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- server/data/supabaseTypes.ts
- server/supabase/migrations/20240609010604_helper_party_api_keys.sql
Stacked diff: I'll update the base and rebase manually, but I put the base on here so that the diff is easy to review.
This creates a new table
helper_party_api_keys, which will be used for helper parties to report their status back to the management plane (on the path to being able to manage the query queue, and start a new query when one finishes.)Summary by CodeRabbit
Chores
.gitignoreto ignore.env*.localfiles for better handling of local environment configurations.New Features
Database
helper_party_api_keystable with columns for UUID, references, hashed API keys, timestamps, and modification reasons.helper_party_api_keystable.