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

Add anonymous scopes to services #3704

Closed

Conversation

helfi92
Copy link
Contributor

@helfi92 helfi92 commented Oct 20, 2020

Relates to #3615.

@helfi92 helfi92 requested a review from a team as a code owner October 20, 2020 18:30
@helfi92 helfi92 self-assigned this Oct 20, 2020
@helfi92 helfi92 requested review from imbstack and jwhitlock and removed request for a team October 20, 2020 18:30
Copy link
Contributor

@jwhitlock jwhitlock left a comment

Choose a reason for hiding this comment

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

Thanks @helfi92. This looks good. I don't understand the significance of adding this scope, so maybe @imbstack should +1 before merging.

@@ -3,3 +3,4 @@
- queue:route:statuses
- queue:route:checks
- assume:scheduler-id:taskcluster-github/*
- assume:anonymous
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: git will complain without a newline at the end of the file, here and other places.

Copy link
Contributor

@imbstack imbstack left a comment

Choose a reason for hiding this comment

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

In the context of this line why do we need to add this? Do the static clients not get that scope added?

@helfi92 helfi92 force-pushed the rfc165 branch 3 times, most recently from 83d24aa to f3fb211 Compare October 23, 2020 23:37
@djmitche djmitche mentioned this pull request Oct 23, 2020
Copy link
Collaborator

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

I don't think this is necessary, based onhttps://github.com//pull/3615#issuecomment-715636601.

I suspect this PR was created because of #3691, with the idea that if one service needs this scope, they all do.

I think the only reason that #3691 was required was that #3616 landed before the rest of the RFC#165 work in #3615. In fact, I think that both PRs become redundant after #3615 lands and can be reverted. If #3615 is correct, then we never need to specify assume:anonymous anywhere, because it's implicitly included everywhere (including in auth.currentScopes which the frontend uses for the Profile view).

So to be clear, my thinking is:

@djmitche
Copy link
Collaborator

--> #3801.

@djmitche djmitche closed this Oct 28, 2020
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.

None yet

4 participants