-
Notifications
You must be signed in to change notification settings - Fork 66
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cfryanr
force-pushed
the
refactor_supervisor_authenticators
branch
from
February 14, 2024 00:40
78b0a54
to
34e4932
Compare
cfryanr
force-pushed
the
refactor_supervisor_authenticators
branch
2 times, most recently
from
February 16, 2024 18:02
ce40af7
to
64be06a
Compare
- 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
force-pushed
the
refactor_supervisor_authenticators
branch
from
February 20, 2024 17:26
035e48c
to
38094e8
Compare
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
force-pushed
the
refactor_supervisor_authenticators
branch
from
February 20, 2024 18:46
38094e8
to
b341e52
Compare
cfryanr
changed the title
WIP: refactoring
Refactor Supervisor to make interface for upstream IDPs, to better separate upstream and downstream concerns
Feb 20, 2024
internal/federationdomain/endpoints/callback/callback_handler.go
Outdated
Show resolved
Hide resolved
benjaminapetersen
approved these changes
Feb 20, 2024
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, nice refactor!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
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
andFederationDomainIdentityProvidersListerI
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 inresolved_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
filesresolved_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 newtestidplister
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.