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

OIDC: Follow-up tasks #12915

Merged
merged 34 commits into from
Feb 22, 2023
Merged

OIDC: Follow-up tasks #12915

merged 34 commits into from
Feb 22, 2023

Conversation

woodruffw
Copy link
Member

@woodruffw woodruffw commented Jan 31, 2023

WIP; I'm going to use this PR to polish off all the remaining tasks listed in #11296.

Specific tasks:

  • Actually handling pending OIDC providers inside of mint-token
  • The provider pages should list existing providers first, followed by form(s) to add new providers
  • Both provider types should be listed under /manage/account/publishing
    - [ ] The admin app should provide a list of pending providers, and possibly some functionality for administering them

Closes #11296.

@woodruffw woodruffw self-assigned this Jan 31, 2023
@woodruffw
Copy link
Member Author

woodruffw commented Feb 1, 2023

  • Both provider types should be listed under /manage/account/publishing

Hmm, this is going to be a little annoying: "normal" OIDC providers are conceptually untied to individual users and can be spread across multiple projects with disjoint maintainer sets, so removing a normal OIDC provider from the account view isn't well-defined: it isn't clear whether it means "remove the OIDC provider entirely" (probably not, since that might disable the provider for projects the user doesn't have permission to modify), or whether it means "remove the OIDC provider from some specific project" (probably, but then we need to duplicate the provider row N times for each project it occurs in).

The solution here might be to stop being overly clever with the OIDC provider data relations: rather than having a many-many relationship between [Project] <-> [OIDCProvider], we could ratchet things down to Project -> OIDCProvider and allow "duplicate" OIDC providers as duplicate DB entries. This would then mean that every project conceptually only has a single valid OIDC provider, although "equivalent" OIDC providers can be registered to multiple projects at once.


Edit: Actually, that doesn't work either: we need the provider -> [project] relationship to mint tokens with the right caveats.


Edit: Thinking about this more, maybe we can avoid the provider -> [project] relationship by bridging the gap at the claim and services level: rather than have each provider track the list of projects its registered to, each project has a single (non-unique) project registered to it. Then, the OIDCProviderService can do a claims -> [project] lookup that's completely transparent in terms of the underlying project/provider relationships.

The downside would then be that there'd be no OIDCProvider to directly record in events, which isn't great.


Edit: Or maybe all of this could be sidestepped by not trying to shoehorn the two different provider types into the exact same table -- I could just have them be two tables, one on top of the other (with the "normal" one not having delete buttons because it doesn't make sense in the account management view).

@woodruffw
Copy link
Member Author

9af0e13 has a rough sketch of the pending provider conversion -- we search for a pending provider matching the JWT, create a project for it if it doesn't exist, and then use that newly created project to register a "reified" copy of the provider so that we can subsequently issue a temporary API token.

warehouse/oidc/views.py Outdated Show resolved Hide resolved
@woodruffw
Copy link
Member Author

woodruffw commented Feb 7, 2023

Edit: Or maybe all of this could be sidestepped by not trying to shoehorn the two different provider types into the exact same table -- I could just have them be two tables, one on top of the other (with the "normal" one not having delete buttons because it doesn't make sense in the account management view).

Going with this one.

Edit: e161633



def reify_pending_provider(
session, pending_provider: PendingOIDCProvider, remote_addr: str
Copy link
Member Author

Choose a reason for hiding this comment

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

Flagging for review: I plumbed the remote_addr through here since we need it for the various events we create, but it's a little out of place. The other option I could think of was plumbing the entire request in, but that felt a little strange as well (given that this util is called from the OIDCService).

@woodruffw
Copy link
Member Author

This changeset is growing rapidly, so I'll do the admin app changes in another follow-up.

@woodruffw woodruffw marked this pull request as ready for review February 9, 2023 17:55
@woodruffw woodruffw requested a review from a team as a code owner February 9, 2023 17:55
@woodruffw
Copy link
Member Author

Lint is failing, for reasons that aren't immediately obvious to me:

+ python -m mypy -p warehouse
warehouse/oidc/models.py:328: error: Unexpected keyword argument "repository_name" for "GitHubProvider"
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "GitHubProvider" defined here
warehouse/oidc/models.py:328: error: Unexpected keyword argument "repository_owner" for "GitHubProvider"
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "GitHubProvider" defined here
warehouse/oidc/models.py:328: error: Unexpected keyword argument "repository_owner_id" for "GitHubProvider"
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "GitHubProvider" defined here
warehouse/oidc/models.py:328: error: Unexpected keyword argument "workflow_filename" for "GitHubProvider"
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "GitHubProvider" defined here
warehouse/oidc/models.py:328: error: Unexpected keyword argument "repository_name" for "GitHubProvider"
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "GitHubProvider" defined here
warehouse/oidc/models.py:328: error: Unexpected keyword argument "repository_owner" for "GitHubProvider"
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "GitHubProvider" defined here
warehouse/oidc/models.py:328: error: Unexpected keyword argument "repository_owner_id" for "GitHubProvider"
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "GitHubProvider" defined here
warehouse/oidc/models.py:328: error: Unexpected keyword argument "workflow_filename" for "GitHubProvider"
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "GitHubProvider" defined here
warehouse/oidc/utils.py:69: error: "Type[Model]" has no attribute "workflow_filename"
warehouse/oidc/utils.py:93: error: Unexpected keyword argument "name" for "Project"
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "Project" defined here
warehouse/oidc/utils.py:97: error: Unexpected keyword argument "name" for "JournalEntry"
warehouse/oidc/utils.py:100: error: "PendingOIDCProvider" has no attribute "added_by"; maybe "added_by_id"?
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "JournalEntry" defined here
warehouse/oidc/utils.py:97: error: Unexpected keyword argument "action" for "JournalEntry"
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "JournalEntry" defined here
warehouse/oidc/utils.py:97: error: Unexpected keyword argument "submitted_by" for "JournalEntry"
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "JournalEntry" defined here
warehouse/oidc/utils.py:97: error: Unexpected keyword argument "submitted_from" for "JournalEntry"
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "JournalEntry" defined here
warehouse/oidc/utils.py:108: error: "PendingOIDCProvider" has no attribute "added_by"; maybe "added_by_id"?
warehouse/oidc/utils.py:112: error: Unexpected keyword argument "user" for "Role"
warehouse/oidc/utils.py:112: error: "PendingOIDCProvider" has no attribute "added_by"; maybe "added_by_id"?
/opt/hostedtoolcache/Python/3.11.1/x[64](https://github.com/pypi/warehouse/actions/runs/4137032670/jobs/7151688458#step:9:65)/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "Role" defined here
warehouse/oidc/utils.py:112: error: Unexpected keyword argument "project" for "Role"
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "Role" defined here
warehouse/oidc/utils.py:112: error: Unexpected keyword argument "role_name" for "Role"
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "Role" defined here
warehouse/oidc/utils.py:116: error: Unexpected keyword argument "name" for "JournalEntry"
warehouse/oidc/utils.py:118: error: "PendingOIDCProvider" has no attribute "added_by"; maybe "added_by_id"?
warehouse/oidc/utils.py:119: error: "PendingOIDCProvider" has no attribute "added_by"; maybe "added_by_id"?
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "JournalEntry" defined here
warehouse/oidc/utils.py:116: error: Unexpected keyword argument "action" for "JournalEntry"
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "JournalEntry" defined here
warehouse/oidc/utils.py:116: error: Unexpected keyword argument "submitted_by" for "JournalEntry"
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "JournalEntry" defined here
warehouse/oidc/utils.py:116: error: Unexpected keyword argument "submitted_from" for "JournalEntry"
/opt/hostedtoolcache/Python/3.11.1/x64/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "JournalEntry" defined here
warehouse/oidc/utils.py:127: error: "PendingOIDCProvider" has no attribute "added_by"; maybe "added_by_id"?
warehouse/oidc/utils.py:129: error: "PendingOIDCProvider" has no attribute "added_by"; maybe "added_by_id"?
warehouse/oidc/utils.py:134: error: "Project" has no attribute "oidc_providers"

@woodruffw woodruffw requested a review from di February 9, 2023 18:02
@woodruffw
Copy link
Member Author

In particular, this doesn't make sense to me:

warehouse/oidc/utils.py:116: error: Unexpected keyword argument "action" for "JournalEntry"
/opt/warehouse/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "JournalEntry" defined here
warehouse/oidc/utils.py:116: error: Unexpected keyword argument "submitted_by" for "JournalEntry"
/opt/warehouse/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "JournalEntry" defined here
warehouse/oidc/utils.py:116: error: Unexpected keyword argument "submitted_from" for "JournalEntry"
/opt/warehouse/lib/python3.11/site-packages/mypy/typeshed/stdlib/builtins.pyi:90: note: "JournalEntry" defined here

That's from a JournalEntry(...) ctor call, but it's identical to other parts of the codebase (like forklift/legacy.py) where mypy doesn't complain. As far as I can tell, it isn't configured to ignore that file, either.

warehouse/oidc/utils.py Outdated Show resolved Hide resolved
Have Pydantic do all the lifting.

Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
This reverts commit e0cdd6a1336ec1f19e2180637c42253ea277f180.
@di di force-pushed the tob-pending-oidc-followup branch from 00ac057 to 1ccdccd Compare February 16, 2023 22:45
warehouse/forklift/legacy.py Outdated Show resolved Hide resolved
warehouse/oidc/models.py Outdated Show resolved Hide resolved
warehouse/oidc/utils.py Outdated Show resolved Hide resolved
warehouse/oidc/models.py Show resolved Hide resolved
warehouse/templates/manage/account/publishing.html Outdated Show resolved Hide resolved
@woodruffw
Copy link
Member Author

#13018 is merged, so I'll update here and resume.

@woodruffw woodruffw requested a review from di February 22, 2023 16:26
woodruffw and others added 3 commits February 22, 2023 11:27
Orphaned pending providers are now pruned during minting.

Signed-off-by: William Woodruff <[email protected]>
@di di merged commit 29e8063 into pypi:main Feb 22, 2023
@di di deleted the tob-pending-oidc-followup branch February 22, 2023 17:57
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.

"De novo" project creation via API tokens/OIDC
2 participants