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

Refactor Supervisor to make interface for upstream IDPs, to better separate upstream and downstream concerns #1867

Merged
merged 6 commits into from
Feb 21, 2024

Conversation

cfryanr
Copy link
Member

@cfryanr cfryanr commented Feb 13, 2024

This PR is a large refactoring of the Pinniped Supervisor code with (almost) no behavior change. It more clearly separates upstream (conversation with external identity providers) and downstream (conversion with the Pinniped CLI or 3rd-party OIDC clients) concerns.

It adds a new interface functions to represent any upstream identity provider on a FederationDomain. The new interface is in resolved_provider.go. This new interface is (mostly) only concerned with upstream identities, so implementations do not need to be concerned with identity transformations, session storage, or other concerns common to all upstream IDP types.

All of the Supervisor endpoints are updated to use this interface, thus hiding all details about the upstream identity providers from the endpoint handling code. This includes the following endpoints:

  • IDP discovery endpoint
  • IDP chooser UI endpoint
  • authorization endpoint
  • callback endpoint (for OIDC upstreams)
  • token endpoint
  • POST login endpoint (for LDAP/AD upstreams)

Note that there are no changes to the discovery endpoint, the JWKS endpoint, or aggregated API endpoints because those endpoints do not need any knowledge of upstream IDPs.

The FederationDomainIdentityProvidersFinderI and FederationDomainIdentityProvidersListerI interfaces are now more polymorphic in style. They can always return one type (the new interface) instead of returning multiple types (one for each upstream IDP type). These are used by the endpoint handlers when the endpoints need to decide which upstream IDP to use for a specific request.

The upstream code for LDAP/AD logins and refreshes is now centralized in resolved_ldap_provider.go. The upstream code for OIDC logins and refreshes is now centralized in resolved_oidc_provider.go.

All of the above changes should make it easier to add support for more upstream identity providers in the future, e.g. as proposed in #1859.

There is purposefully minimal changes to the tests, because this is intended to be (mostly) a "pure" refactoring. The facts that the tests did not change and the tests still pass prove that there is no behavior change. In the future, we may wish to add some unit tests for the new resolved_ldap_provider.go files resolved_oidc_provider.go, which are currently entirely covered indirectly by the unit tests of the various endpoints. Because the unit tests of the endpoints did not mock the new interface, they provide confidence that this refactoring is correct. On the other hand, if they did mock at the new interface they could be a lot simpler (would not need to understand differences between different upstream IDP types). These tests are not refactored in this PR, and it's not clear if they should be refactored, because they are giving us such powerful and useful test coverage (almost integration-like). This is a philosophical question of test style, but the "thick" test style currently used in the unit tests of the various endpoints strongly benefited (entirely made possible) this refactor.

One change made to the tests was to split up the code in the oidctestutil package into separate files to make it easier to group things together more logically. Some of it moved to a new testidplister package to avoid circular dependencies in tests.

The only behavior change is a small change in how the authorize endpoint returns certain uncommon errors. Some types of errors were previously returned as regular http-style errors. Those have all been converted to be returned as oauth-style errors (which can be redirects to the client), except for http method not found errors. This is a change in behavior from the client's point of view, but only when those unexpected errors happen. These types of errors are more consistent with RFC6749 section 4.1.2.1.

@cfryanr cfryanr marked this pull request as draft February 13, 2024 23:42
Copy link

codecov bot commented Feb 13, 2024

Codecov Report

Attention: 536 lines in your changes are missing coverage. Please review.

Comparison is base (23dce42) 38.68% compared to head (4b4a4ad) 38.12%.

Files Patch % Lines
...vedprovider/resolvedoidc/resolved_oidc_provider.go 3.22% 420 Missing ⚠️
...vedprovider/resolvedldap/resolved_ldap_provider.go 73.42% 32 Missing and 6 partials ⚠️
...tiondomain/downstreamsession/downstream_session.go 3.12% 31 Missing ⚠️
internal/testutil/oidctestutil/testoidcprovider.go 93.02% 13 Missing and 5 partials ⚠️
...rationdomain/endpoints/login/post_login_handler.go 82.50% 6 Missing and 1 partial ⚠️
...util/oidctestutil/expected_upstream_state_param.go 72.72% 6 Missing ⚠️
internal/testutil/oidctestutil/testldapprovider.go 92.00% 5 Missing and 1 partial ⚠️
...l/federationdomain/endpoints/loginurl/login_url.go 76.92% 2 Missing and 1 partial ⚠️
internal/testutil/oidctestutil/verify_id_token.go 83.33% 2 Missing and 1 partial ⚠️
...al/federationdomain/endpoints/auth/auth_handler.go 98.68% 1 Missing and 1 partial ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1867      +/-   ##
==========================================
- Coverage   38.68%   38.12%   -0.57%     
==========================================
  Files         335      346      +11     
  Lines       43626    43636      +10     
==========================================
- Hits        16876    16635     -241     
- Misses      26246    26486     +240     
- Partials      504      515      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cfryanr cfryanr force-pushed the refactor_supervisor_authenticators branch from 78b0a54 to 34e4932 Compare February 14, 2024 00:40
@cfryanr cfryanr force-pushed the refactor_supervisor_authenticators branch 2 times, most recently from ce40af7 to 64be06a Compare February 16, 2024 18:02
- Simplify the error handling in the authorize endpoint by making the
  private helper functions return fosite-style errors, and having
  one place that writes those errors to the response.
- Some types of errors were previously returned as regular http-style
  errors. Those have all been converted to be returned as oauth-style
  errors (which can be redirects to the client), except for http method
  not found errors. This is a change in behavior from the client's point
  of view, but only when those unexpected errors happen. These types of
  errors are more consistent with RFC6749 section 4.1.2.1.
- Avoids using the httperr package for error handling.
- Create a struct for the handler as a first step toward making smaller
  functions with fewer parameters.
- continued refactoring the auth handler to share more code between
  the two supported browserless flows: OIDC and LDAP/AD
- the upstreamldap package should not know about the concept of
  OIDC granted scopes, so refactored it to be a skipGroups bool
Create an interface to abstract the upstream IDP from the
authorize, IDP discovery, callback, choose IDP, and login
endpoints. This commit does not refactor the token endpoint,
which will be refactored in a similar way in the next commit.
@cfryanr cfryanr force-pushed the refactor_supervisor_authenticators branch from 035e48c to 38094e8 Compare February 20, 2024 17:26
Each endpoint handler is now responsible for applying the identity
transformations and creating most of the session data, rather than each
implementation of the upstream IDP interface. This shares code better,
and reduces the responsibilities of the implementations of the IDP
interface by letting them focus more on the upstream stuff.

Also refactor the parameters and return types of the IDP interfaces to
make them more clear, and because they can be more focused on upstream
identities (pre-identity transformation). This clarifies the
responsibilities of the implementations of the IDP interface.
@cfryanr cfryanr force-pushed the refactor_supervisor_authenticators branch from 38094e8 to b341e52 Compare February 20, 2024 18:46
@cfryanr cfryanr changed the title WIP: refactoring Refactor Supervisor to make interface for upstream IDPs, to better separate upstream and downstream concerns Feb 20, 2024
@cfryanr cfryanr marked this pull request as ready for review February 20, 2024 18:52
Copy link
Member

@benjaminapetersen benjaminapetersen left a comment

Choose a reason for hiding this comment

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

LGTM, nice refactor!

@cfryanr cfryanr merged commit 867468e into main Feb 21, 2024
39 checks passed
@cfryanr cfryanr deleted the refactor_supervisor_authenticators branch February 21, 2024 06:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants