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

chore(auth): remove max scope for partners #82437

Closed
wants to merge 1 commit into from

Conversation

sentaur-athena
Copy link
Member

I added this piece of code a couple of weeks ago because we wanted to limit partners max scope but we decided not to. Removing it because it adds extra complexity

@sentaur-athena sentaur-athena requested review from mdtro and a team December 19, 2024 23:47
@sentaur-athena sentaur-athena requested a review from a team as a code owner December 19, 2024 23:47
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Dec 19, 2024
Copy link
Member

@GabeVillalobos GabeVillalobos left a comment

Choose a reason for hiding this comment

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

looks good to me, but I also don't fully understand the implications of removing this so I'll defer to you + security on it.

@sentaur-athena
Copy link
Member Author

@GabeVillalobos it's a product decision we made initially but then in practice it ended up being too daunting for a partner. The scopes no matter what partner requests for will be visible to the user and user can decide if that's too much scope and they don't want to grant.

@mdtro
Copy link
Member

mdtro commented Dec 19, 2024

@sentaur-athena Is it necessary to remove this? For partners that need the max scopes, wouldn't we just check all the scopes? That way, with cases where the scope does need a subset of the max it can be restricted still.

@sentaur-athena sentaur-athena enabled auto-merge (squash) December 19, 2024 23:56
@sentaur-athena
Copy link
Member Author

@mdtro I'm open to keeping it but it creates complexity that we don't have a reason for right now.

I realized we keep defining new scopes, like alerts scopes are create just a couple of weeks ago and member:invite scope. Having this logic will enforce us to backfill every time a new scope gets created.

Other solution would be to keep the max scope but if it's empty we assume it's all. 😅

@sentaur-athena sentaur-athena enabled auto-merge (squash) December 20, 2024 00:00
Copy link

codecov bot commented Dec 20, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
23356 1 23355 213
View the top 1 failed tests by shortest run time
tests.sentry.sentry_metrics.test_snuba.SnubaMetricsInterfaceTest::test_count_query
Stack Traces | 3.4s run time
#x1B[1m#x1B[.../sentry/sentry_metrics/test_snuba.py#x1B[0m:57: in test_count_query
    data = get_series(
#x1B[1m#x1B[.../snuba/metrics/datasource.py#x1B[0m:1071: in get_series
    ).get_snuba_queries()
#x1B[1m#x1B[.../snuba/metrics/query_builder.py#x1B[0m:1133: in get_snuba_queries
    component_entities = metric_field_obj.get_entity(
#x1B[1m#x1B[.../metrics/fields/base.py#x1B[0m:784: in get_entity
    return _get_entity_of_metric_mri(projects, self.metric_object.metric_mri, use_case_id).value
#x1B[1m#x1B[.../metrics/fields/base.py#x1B[0m:278: in _get_entity_of_metric_mri
    raise InvalidParams(f"Raw metric {get_public_name_from_mri(metric_mri)} does not exist")
#x1B[1m#x1B[31mE   sentry.exceptions.InvalidParams: Raw metric measurements.speed does not exist#x1B[0m

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

@sentaur-athena
Copy link
Member Author

I realized letting partners to have max scope (even the future ones created) is more code change that I want to make today before holidays. Will close this for now but that's the route I will go when back from holidays.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants