Repository navigation
Conversation
f0d7e45 to
1ff553f
Compare
|
Okay, useful input, thanks gents @haacked & @benjackwhite 🎩 What if we go about it this way - only allow this key type (SDK naming TBC) on specific actions that make our customers' apps run, i.e. endpoint execution, FF local evaluation, and such. Configuration (CRUD) is done by humans, execution by machines. This way, we exclusively specify the |
|
The model LGTM and covers the feedback I shared in https://github.com/PostHog/product-internal/pull/843. Will leave the more important permissioning points to the folks with more context. |
What's "endpoint execution" in this context? |
|
Size Change: +697 B (+0.02%) Total Size: 3.7 MB ℹ️ View Unchanged
|
@haacked, I'm referring to the new Endpoints product that we're building. Essentially a pre-defined query meant to power B2B2C dashboards. Endpoint execution is the /api/projects/2/endpoints/event-count/run route. See the changed files in products/endpoints/backend of this PR for more. |
58175cc to
4194f11
Compare
sakce
left a comment
There was a problem hiding this comment.
Refactored without touching the TeamAndOrgViewSet 👀
| authentication_classes=[ProjectSecretAPIKeyAuthentication], | ||
| permission_classes=[ProjectSecretAPIKeyPermission], |
There was a problem hiding this comment.
This is the key difference - we only set this on the action we want.
This comment was marked as off-topic.
This comment was marked as off-topic.
b50c23b to
c8e1b06
Compare
|
How strongly do customers want this? I can totally see the benefit, but I think there is also a major drawback to introducing new keys for users to understand, which I've already seen happening with the feature flags key we introduced. We could make the UX of creating personal api keys for a team much better, or even create them by default for a team. We could also handle transferring the team scoped keys in the UX for removing a team member from a team to avoid the user leaves and everything breaks problem. Having tokens team scoped feels like the right thing to do, but not sure if it's worth the added complexity for users and for us if there are other ways around it to keep |
benjackwhite
left a comment
There was a problem hiding this comment.
Generally this is a fantastic PR. Some changes I think we need to make and then we can roll with it
| capture_exception(e) | ||
| raise ValidationError("Failed to create project secret API key.") | ||
|
|
||
| def validate_scopes(self, scopes): |
There was a problem hiding this comment.
This kind of thing is why I prefer the DRF serializers as it makes it more encapsulated. Not blocking but the code in this viewset feels much more verbose than the personal_api_key one for example which is very similar...
There was a problem hiding this comment.
im with you on verbosity - I'll 📎 this for cleanup in a subsequent PR.
There was a problem hiding this comment.
And maybe we get Julian to give input on use of PydanticModelMixin by then
ea0a90b to
fa2245b
Compare
I don't think it would. I can try and test this branch out soon, but we have a lot going on right now so probably won't get to it immediately. |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
Visual regression: Storybook UI snapshots updatedChanges: 2 snapshots (2 modified, 0 added, 0 deleted) What this means:
Next steps:
|
62b50bc to
f2fb85f
Compare
|
Cursor Agent can help with this pull request. Just |
25dd9d5 to
b780509
Compare
There was a problem hiding this comment.
9 issues found across 37 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/queries/schema.json">
<violation number="1" location="frontend/src/queries/schema.json:21400">
P2: The `scopes` array items should reference `ProjectSecretAPIKeyAllowedScope` enum to ensure proper schema validation. Currently any string is accepted.</violation>
</file>
<file name="products/endpoints/backend/api.py">
<violation number="1" location="products/endpoints/backend/api.py:205">
P1: Logic bug: changing from `is not None` to truthiness check breaks `is_active=False` handling. When a user explicitly sets `is_active=False`, this condition evaluates the falsy `False` value and incorrectly defaults to `True`. The original check `data.is_active is not None` was correct.</violation>
</file>
<file name="posthog/permissions.py">
<violation number="1" location="posthog/permissions.py:496">
P1: Missing null check for `required_scopes` before passing to `_check_required_scope_satisfied`. When `_get_required_scopes` returns `None`, this will raise a `TypeError` when iterating. The existing `APIScopePermission` class handles this case properly.</violation>
</file>
<file name="posthog/api/project_secret_api_key.py">
<violation number="1" location="posthog/api/project_secret_api_key.py:156">
P2: Update method allows setting an empty label when whitespace-only strings are passed. The create method properly validates `if not data.label or not data.label.strip()`, but update only checks `if data.label:` before stripping. A whitespace-only label like `" "` would pass the truthy check but strip to an empty string.</violation>
<violation number="2" location="posthog/api/project_secret_api_key.py:203">
P2: Inconsistent use of `get_object_or_404` instead of `self.get_object()`. Other methods (retrieve, update, roll) use the standard `self.get_object()` which respects DRF's built-in permission checks and queryset filtering. Using `get_object_or_404` directly bypasses this standard flow.</violation>
</file>
<file name="posthog/api/test/test_project_secret_api_keys.py">
<violation number="1" location="posthog/api/test/test_project_secret_api_keys.py:286">
P2: Test is incomplete: missing status code assertion and verification that the key actually changed. Consider capturing the original `mask_value` before rolling and asserting it differs afterward, plus adding a status code check.</violation>
</file>
<file name="frontend/src/scenes/settings/project/ProjectAPIKeys.tsx">
<violation number="1" location="frontend/src/scenes/settings/project/ProjectAPIKeys.tsx:116">
P3: Grammatical error: 'allow programmatic access a very limited set' is missing 'to'. Should be 'allow programmatic access to a very limited set'.</violation>
<violation number="2" location="frontend/src/scenes/settings/project/ProjectAPIKeys.tsx:120">
P3: Grammatical error: 'scopes that allow to the project's data' should be 'scopes that allow access to the project's data'.</violation>
</file>
<file name="frontend/src/scenes/settings/shared/APIKeyTable.tsx">
<violation number="1" location="frontend/src/scenes/settings/shared/APIKeyTable.tsx:132">
P1: Logic is inverted: `actionsDisabled` being `false` (default) hides the actions column, while `true` shows it. The condition should be `!actionsDisabled` to match the prop name semantics.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
f1a8dd3 to
d88a134
Compare
Visual regression: Storybook UI snapshots updatedChanges: 2 snapshots (2 modified, 0 added, 0 deleted) What this means:
Next steps:
|
968d169 to
49343ac
Compare
haacked
left a comment
There was a problem hiding this comment.
Let me take some more time to look at this. But found a few issues worth looking at.
49343ac to
0762510
Compare
0762510 to
e21875b
Compare
## Problem <!-- Who are we building for, what are their needs, why is this important? --> <!-- Does this fix an issue? Uncomment the line below with the issue ID to automatically close it when merged --> <!-- Closes #ISSUE_ID --> Splits #39865 - second part. API and authenticator changes. ## Changes <!-- If there are frontend changes, please include screenshots. --> <!-- If a reference design was involved, include a link to the relevant Figma frame! --> - adds an authenticator class for ProjectSecretAPIKey - adjusts the permissions - adds the API for project secret API key (pretty much the same thing as personal api key) ## How did you test this code? - locally <!-- Briefly describe the steps you took. --> <!-- Include automated tests if possible, otherwise describe the manual testing routine. --> <!-- Docs reminder: If this change requires updated docs, please do that! Engineers are the primary people responsible for their documentation. 🙌 --> 👉 _Stay up-to-date with [PostHog coding conventions](https://posthog.com/docs/contribute/coding-conventions) for a smoother review._ ## Publish to changelog? <!-- For features only --> <!-- If publishing, you must provide changelog details in the #changelog Slack channel. You will receive a follow-up PR comment or notification. --> <!-- If not, write "no" or "do not publish to changelog" to explicitly opt-out of posting to #changelog. Removing this entire section will not prevent posting. --> no

Problem
Customers have explicitly asked for project-level API keys, so when their employees leave the company their API keys are not breaking their apps.
But we also already have a Feature Flags use case that has a token that is not tied to a user. (long term, we would move that over to project secret API keys)
Closes #843
Changes
Introducing a ProjectSecretAPIKey model, along with a new Permission that only allows specific scopes (we don't want this key type to allow all endpoints, read the discussion in the RFC)
Also implements this auth/permission on the Endpoints model, so that we see it in action. Will happily break this out into a separate PR after validating approach here.
Pretty much clones the frontend approach we have for PersonalAPIKeys (we can refactor PersonalAPIKeys to reuse this later on).
How did you test this code?
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Changelog: (features only) Is this feature complete?