Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add service account checks in plugin auth #5305

Merged
merged 2 commits into from
Nov 28, 2024

Conversation

matiasb
Copy link
Contributor

@matiasb matiasb commented Nov 28, 2024

Related to https://github.com/grafana/oncall-private/issues/2826
Related to https://github.com/grafana/irm/pull/459

Allow org sync requests from service account users. Also trigger a sync during public API requests if the org wasn't yet setup.

@matiasb matiasb added pr:no public docs Added to a PR that does not require public documentation updates release:patch PR will be added to "Other Changes" section of release notes labels Nov 28, 2024
@matiasb matiasb force-pushed the matiasb/service-account-plugin-auth-check branch from bfcc21c to 6b69970 Compare November 28, 2024 13:33
@matiasb matiasb marked this pull request as ready for review November 28, 2024 14:39
@matiasb matiasb requested a review from a team as a code owner November 28, 2024 14:39
@@ -133,6 +134,14 @@ def _get_user(request: Request, organization: Organization) -> User:
except KeyError:
user_id = context["UserID"]

if context.get("IsServiceAccount", False):
Copy link
Contributor

Choose a reason for hiding this comment

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

This just reminded me we need to clean-up the GrafanaHeadersMixin in mixins.py at some point whenever we remove the old status endpoint. This way of doing it is better in that it is more tolerant of missing/different fields when dealing with plugin rollout difference between objects.

Copy link
Contributor

@joeyorlando joeyorlando left a comment

Choose a reason for hiding this comment

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

LGTM 👍

grafana_url = request.headers.get(X_GRAFANA_URL)
if grafana_url:
organization = Organization.objects.filter(grafana_url=grafana_url).first()
if not organization:
raise exceptions.AuthenticationFailed("Invalid Grafana URL.")
success = setup_organization(grafana_url, auth)
Copy link
Contributor

Choose a reason for hiding this comment

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

just out of curiosity, what's the use-case for the new setup_organization call here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a use case in which you can bootstrap a Grafana stack from scratch (via Terraform), setting up a service account token in the process with which you could hit our API and OnCall may not know about the organization yet, so this should sync the org if the service account token auth passes and we don't have a record for that org yet.

@joeyorlando joeyorlando added this pull request to the merge queue Nov 28, 2024
Merged via the queue into dev with commit bb4875f Nov 28, 2024
26 checks passed
@joeyorlando joeyorlando deleted the matiasb/service-account-plugin-auth-check branch November 28, 2024 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates release:patch PR will be added to "Other Changes" section of release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants