-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Generalize trusted publishing emails #13872
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
Conversation
Signed-off-by: William Woodruff <[email protected]>
This makes the `trusted-publisher-added` and `trusted-publisher-removed` email structures a little more generic, allowing them to be re-used over both GitHub and Google publishers. Future publishers will require additional accommodations. See pypi#13551. Signed-off-by: William Woodruff <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM aside from that one comment.
warehouse/oidc/models/google.py
Outdated
def publisher_url(self, claims=None): | ||
return "https://accounts.google.com" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For GitHub, this ends up being a link to a specific repository or SHA. I'm not sure this is useful for Google accounts. If these were always true Google accounts, we could link to https://accounts.google.com/[email protected]
, but I don't think that will work for service accounts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there may not be a great URL here. Perhaps we could link to Google's OIDC docs instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should really only provide this if it's specific to the identity used. I think it's probably fine for this to return None
for now, and for the templates to handle this accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, will update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, updated -- I updated a handful of the uses of publisher_url()
to prepare them for providers where it'll be None
, but some rendering uses will require a more in-depth look with the view/form changes (e.g. the trusted publisher table on each user/project).
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
@woodruffw You've got a failing test here now. |
Signed-off-by: William Woodruff <[email protected]>
Thanks, fixed! |
This makes the
trusted-publisher-added
andtrusted-publisher-removed
email structures a little moregeneric, allowing them to be re-used over both
GitHub and Google publishers. Future publishers will require
additional accommodations.
See #13551.
Signed-off-by: William Woodruff [email protected]