Skip to content

Add a step to the crashlytics:web:onboard command to grant read acces… - #10963

Draft
bryanatkinson wants to merge 2 commits into
mainfrom
crashlytics-log-bucket-onboarding
Draft

Add a step to the crashlytics:web:onboard command to grant read acces…#10963
bryanatkinson wants to merge 2 commits into
mainfrom
crashlytics-log-bucket-onboarding

Conversation

@bryanatkinson

Copy link
Copy Markdown
Contributor

Add a step to the crashlytics:web:onboard command to grant read access to the newly created firebase-telemetry log bucket to project viewers and editors. They always have this access to the _Default bucket, but this needs to be done explicitly for custom buckets like this.

…s to the newly created firebase-telemetry log bucket to project viewers and editors. They always have this access to the _Default bucket, but this needs to be done explicitly for custom buckets like this.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request adds support for managing IAM policies on Cloud Logging Log Views and integrates this into the Crashlytics onboarding flow to grant view accessor permissions to project viewers and editors. The feedback highlights a critical issue where thrown FirebaseError instances in getLogViewIamPolicy and setLogViewIamPolicy do not propagate the HTTP status code in their options, causing the status to default to 500 and breaking downstream checks (such as the 404 status check in grantLogViewAccess).

Comment thread src/gcp/cloudlogging.ts
Comment on lines +204 to +207
throw new FirebaseError(
`Failed to get IAM policy for log view ${viewId} on bucket ${bucketId} (status ${err.status}): ${msg}`,
{ original: err },
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The thrown FirebaseError does not propagate the HTTP status code (err.status) in its options. This causes the status property of the resulting FirebaseError to default to 500 (via DEFAULT_STATUS), which breaks downstream status checks (such as err.status === 404 in grantLogViewAccess). Propagating status: err.status in the options ensures the correct status code is preserved.

Suggested change
throw new FirebaseError(
`Failed to get IAM policy for log view ${viewId} on bucket ${bucketId} (status ${err.status}): ${msg}`,
{ original: err },
);
throw new FirebaseError(
`Failed to get IAM policy for log view ${viewId} on bucket ${bucketId} (status ${err.status}): ${msg}`,
{ original: err, status: err.status },
);

Comment thread src/gcp/cloudlogging.ts
Comment on lines +229 to +232
throw new FirebaseError(
`Failed to set IAM policy for log view ${viewId} on bucket ${bucketId} (status ${err.status}): ${msg}`,
{ original: err },
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The thrown FirebaseError does not propagate the HTTP status code (err.status) in its options. This causes the status property of the resulting FirebaseError to default to 500 (via DEFAULT_STATUS). Propagating status: err.status in the options ensures the correct status code is preserved.

Suggested change
throw new FirebaseError(
`Failed to set IAM policy for log view ${viewId} on bucket ${bucketId} (status ${err.status}): ${msg}`,
{ original: err },
);
throw new FirebaseError(
`Failed to set IAM policy for log view ${viewId} on bucket ${bucketId} (status ${err.status}): ${msg}`,
{ original: err, status: err.status },
);

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.

2 participants