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(metrics): Add scopes to Glean access token check event #17847

Merged
merged 1 commit into from
Oct 22, 2024
Merged

Conversation

vbudhram
Copy link
Contributor

Because

  • We want to see the scopes in our Glean events for access token checked

This pull request

  • Adds the scopes as a sorted, comma separated string to those events

Issue that this pull request solves

Closes: https://mozilla-hub.atlassian.net/browse/FXA-10484

Checklist

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Other information (Optional)

@chenba You are probably the best to review this. I wasn't sure if this was the correct way to add an extra value to Glean events. I didn't want to piggy back off the reason value since it didn't seem to make sense in this case.

@vbudhram vbudhram requested a review from a team as a code owner October 17, 2024 15:22
extra_keys:
scopes:
description: The scopes of the access token
type: string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does Glean support Array types? Would that allow for easier sorting and graphs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, the String List type is not support for the extra keys. 😞

@chenba
Copy link
Contributor

chenba commented Oct 18, 2024

@bbangert @vbudhram what do you think of filtering out the common scopes (e.g. 'profile')?

Copy link
Contributor

@chenba chenba left a comment

Choose a reason for hiding this comment

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

Thanks, @vbudhram. LGTM but let's see what Ben think of filtering out the common scopes. I think the intention is to use the scopes to understand what product/client is making the request; the common scopes don't really help with that.

Copy link
Member

@bbangert bbangert left a comment

Choose a reason for hiding this comment

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

LGTM!

@bbangert
Copy link
Member

The common scopes don't help, but I don't think they'll hinder either. However they could result in an increase in cardinality, so filtering out the profile/identity/openid basic ones should be done.

@bbangert
Copy link
Member

bbangert commented Oct 21, 2024

I don't think filtering out the common scopes will matter here, mainly since I believe we'll be doing more specific queries in Looker/etc to quantify the other resource requests. The list of FxA handled scopes can vary greatly over time as well, so keeping those up to date and mapped to 'external' resource servers or not will probably be more time intensive than just adding a Looker view to see access token issuance for all tokens with scope including 'relay', etc.

This list may grow in cardinality over time if clients start doing more specific requests, e.g. read to 2 scopes, write to 3, etc. But should still be a fairly confined list.

Do we have a white-list of all valid scope strings we verify this input on? e.g. this scope set data isn't completely unfiltered client-sent data?

@vbudhram
Copy link
Contributor Author

Do we have a white-list of all valid scope strings we verify this input on? e.g. this scope set data isn't completely unfiltered client-sent data?

Looks like we do store a list in the database and request checks that the scope is valid, so in theory should only see valid scopes in Looker.

@vbudhram vbudhram merged commit 5c1fd7e into main Oct 22, 2024
26 checks passed
@vbudhram vbudhram deleted the fxa-10484 branch October 22, 2024 18:26
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.

3 participants