tests, warehouse: per-provider OIDC admin flags #13871
Conversation
Closes pypi#13841. Signed-off-by: William Woodruff <william@trailofbits.com>
| @@ -1249,7 +1249,7 @@ def add_github_oidc_publisher(self): | |||
| if not self.oidc_enabled: | |||
| raise HTTPNotFound | |||
|
|
|||
| if self.request.flags.enabled(AdminFlagValue.DISALLOW_OIDC): | |||
| if self.request.flags.disallow_oidc(AdminFlagValue.DISALLOW_GITHUB_OIDC): | |||
There was a problem hiding this comment.
I think we should probably make the error message here more accurate, something like "This trusted publisher is temporarily disabled", because the global disable might not be accurate. (This will also potentially help us debug the difference between this error message and other identical ones).
There was a problem hiding this comment.
Makes sense to me -- this particular callsite is GitHub only, so I can make it clear that GitHub-based trusted publishers are disabled here.
| @@ -1366,7 +1366,7 @@ def delete_oidc_publisher(self): | |||
| if not self.oidc_enabled: | |||
| raise HTTPNotFound | |||
|
|
|||
| if self.request.flags.enabled(AdminFlagValue.DISALLOW_OIDC): | |||
| if self.request.flags.disallow_oidc(): | |||
There was a problem hiding this comment.
Should this handle publisher-specific deletions?
There was a problem hiding this comment.
I think it could, although I'm not sure if it's worth the complexity: deletion is conceptually "monomorphic" while creation requires us to use the exact provider variant, so we'd need to pass in additional context with the deletion to figure out which type of instance we're actually deleting (and then accept/reject that deletion request based on the right set of flag values).
So IMO not worth it for the amount of code/special-casing it'll take, but if you'd like to have it then I can figure it out 🙂.
| @@ -1604,7 +1604,7 @@ def delete_pending_oidc_publisher(self): | |||
| if not self.oidc_enabled: | |||
| raise HTTPNotFound | |||
|
|
|||
| if self.request.flags.enabled(AdminFlagValue.DISALLOW_OIDC): | |||
| if self.request.flags.disallow_oidc(): | |||
There was a problem hiding this comment.
Should this handle publisher-specific deletions?
Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
|
@di More generally, what do you think about removing the |
Signed-off-by: William Woodruff <william@trailofbits.com>
Yes let's do it in a follow-on PR. |
Closes #13841.
CC @di: This should be helpful in your form/view work on #13551 🙂