Factor out ORCID-specific code

[seanh]

Depends on https://github.com/hypothesis/devdata/pull/121.

Refactor various unnecessarily ORCID-specific parts of the code to remove ORCID-specific naming and to parametrize hardcoded ORCID-specific values. This paves the way for adding Google and Facebook OIDC support to the same code in future.

Testing

See #9763.

Summary

This is quite a large diff. Summary of the main changes:

Renamed settings and added new settings

I've renamed some of the existing ORCID_* envvars to follow a naming style consistent with how we've been naming other OIDC things (e.g. URLs, route names). Also added some missing settings to replace previously hardcoded ORCID-specific values. The full set of ORCID-related settings is now:

Environment Variable	Description
ORCID_HOST	This is not an OIDC thing. It's just an ORCID thing. It's only used to generate links to users' public ORCID profile pages, e.g. https://orcid.org/0000-0002-6373-1308. In dev and on staging this will use sandbox.orcid.org.
OIDC_CLIENTID_ORCID	Our ORCID client ID. For dev this comes from the devdata repo.
OIDC_CLIENTSECRET_ORCID	Our ORCID client secret. For dev this comes from the devdata repo.
OIDC_AUTHORIZATIONURL_ORCID	The ORCID URL that we redirect the browser to in order to kick off the authorization flow. There was previously no setting for this URL, it was constructed by combining ORCID_HOST with a hard-coded and ORCID-specific path.
OIDC_TOKENURL_ORCID	The URL that we make server-to-server requests to in order to get ID tokens. Again there was previously no setting for this URL, it was constructed by combining ORCID_HOST with a hard-coded and ORCID-specific path.
OIDC_KEYSETURL_ORCID	The URL that we retrieve ORCID's JWT signing public keys from. Again there was previously no setting for this URL, it was constructed by combining ORCID_HOST with a hard-coded and ORCID-specific path.

In future we'll need OIDC_*_GOOGLE and OIDC_*_FACEBOOK versions of all of these.

Services refactoring

On main there are two services ORCIDClientService and OpenIDClientService. ORCIDClientService doesn't actually contain any ORCID-specific code, just ORCID-specific naming and some hard-coded ORCID-specific values that're easily parametrizable. Meanwhile OpenIDClientService contains only one method get_id_token() that's called only once, from ORCIDClientService. This method can easily be inlined into ORCIDClientService and doing so makes the code much clearer.

These two services have been replaced with a new services/oidc.py::OIDCService. The naming is consistent with other parts of the code such as schemas/oidc.py and views/oidc.py. The new OIDCService's interface is as follows:

@dataclass
class OIDCServiceSettings:
    client_id: str
    client_secret: str
    redirect_uri: str
    token_url: str
    keyset_url: str

class OIDCService:
    def __init__(..., settings: dict[IdentityProvider, OIDCServiceSettings], ...):
        ...

    def get_provider_unique_id(self, provider, authorization_code):
        # Makes the server-to-server request to exchange the authorization_code for an
        # ID token, decodes the ID token, and returns the user's ORCID iD
        # (called provider_unique_id to avoid being ORCID-specific).

    def add_identity(self, user, provider, provider_unique_id):
       # Connects the given user's account to the given ORCID iD (provider_unique_id)
       # by adding a UserIdentity to the DB.
       
    def get_identity(self, user, provider):
        # Returns the existing UserIdentity from the DB for user and provider, or None.

def factory(context, request):
    return OIDCService(
        ...,
        settings={
            IdentityProvider.ORCID: OIDCServiceSettings(
                client_id=settings["oidc_clientid_orcid"],
                client_secret=settings["oidc_clientsecret_orcid"],
                redirect_uri=request.route_url("oidc.redirect.orcid"),
                token_url=settings["oidc_tokenurl_orcid"],
                keyset_url=settings["oidc_keyseturl_orcid"],
            ),
            ...
        },
        ...
    )

The settings object passed to OIDCService.__init__() maps providers to their provider-specific settings objects. Each of the three methods OIDCService.get_provider_unique_id(), add_identity(), and get_identity() then takes a provider argument which it uses to look up the provider's settings in self._settings.

Remove route_name predicates from @view_defaults

The code currently groups multiple views and exception views into classes and uses Pyramid's @view_defaults to avoid having to repeat view predicates including route_name:

@view_defaults(..., route_name="foo")
class MyViews:
    @view_config(...) # No need to repeat route_name="foo" here.
    def view1(self):
        ...
    @view_config(...) # No need to repeat route_name="foo" here.
    def view2(self):
        ...

In preparation for adding more OIDC/social login routes to the same views (e.g. "signup.google" and "signup.facebook" in addition to "signup.orcid") remove the route_name predicates from the @view_defaults because it's not possible to have multiple route names in @view_defaults. This doesn't work as you'd hope:

@view_defaults(..., route_name="foo")
@view_defaults(..., route_name="bar")
class MyViews:
    @view_config(...)
    def my_view(self):
        ...

You can't stack multiple @view_defaults decorators on a class. Instead you have to do this:

@view_defaults(...)  # No route_name in the view defaults.
class MyViews:
    @view_config(..., route_name="foo")
    @view_config(..., route_name="bar")
    def my_view(self):
        ...

This does unfortunately lead to some duplication in the @view_config's which could be error prone. A way to avoid the duplication would be to use the add_view() method to configure the views imperatively: https://docs.pylonsproject.org/projects/pyramid/en/latest/narr/viewconfig.html#adding-view-configuration-using-add-view

But I don't think this would be worth the loss of the readability and locality afforded by the @view_defaults and @view_config decorators.

An ideal solution (possibly for a future PR) might be to add our own custom view predicate that allows something like this:

@view_defaults(..., route_names=["foo", "bar"])

https://docs.pylonsproject.org/projects/pyramid/en/latest/narr/hooks.html#adding-a-custom-view-route-or-subscriber-predicate

Views refactoring

views/account_signup.py::ORCIDSignupViews is renamed to SocialSignupViews. (This view class is not an OIDC thing: it's our "Signup with <PROVIDER>" interstitial signup page, which is a custom page that we redirect to after the OIDC flow has finished.)

views/oidc.py::ORCIDConnectAndLoginViews is renamed to OIDCConnectAndLoginViews and views/oidc.py::ORCIDRedirectViews is renamed to OIDCRedirectViews.

Each of these three view classes will handle multiple URLs/routes, one for each provider:

Route	URL	View
oidc.connect.orcid and oidc.login.orcid	/oidc/connect/orcid and /oidc/login/orcid	OIDCConnectAndLoginViews.connect_or_login()
oidc.redirect.orcid	/oidc/redirect/orcid	OIDCRedirectViews.redirect()
signup.orcid	/signup/orcid	SocialSignupViews.get() and post()

In future new routes will be added for new providers, e.g. oidc.connect.google (/oidc/connect/google), oidc.login.google (/oidc/login/google), oidc.redirect.google (/oidc/redirect/google) and signup.google (/signup/google). These *.google and *.facebook routes will be handled by the same views that handle the *.orcid routes but the views will vary the settings they use depending on which route they're handling.

It's important to have a complete set of separate routes for each provider: it's a simple way to ensure that we never get any crossed-wires between providers, and is how the views know which provider they're handling. But separate routes can be handled by the same views.

This has been implemented by having a settings object for each view that contains all provider-specific values needed by that view. Different instances of these settings objects are constructed for different providers depending on the route, and the settings objects are then used by the view code:

@dataclass
class OIDCRedirectViewsSettings:
    """Per-route settings for OIDCRedirectViews."""

    provider_name: str
    ...

@view_defaults(request_method="GET", route_name="oidc.redirect.orcid")
class OIDCRedirectViews:
    def __init__(self, context, request: Request):
        self.context = context
        self.request = request
        ...

    @property
    def settings(self) -> OIDCRedirectViewsSettings:
        """Return the settings for the current route."""
        match self.request.matched_route.name:
            case "oidc.redirect.orcid":
                return OIDCRedirectViewsSettings(
                    provider_name="ORCID",
                    ...
                )
            case _:
                raise UnexpectedRouteName(route_name)

    @view_config()
    def redirect(self):
        # The view uses `self.settings` whenever it needs a route/provider-specific setting.
        state = self.request.session.pop(self.settings.state_session_key)
        ...

[seanh]

This is the CSS for the ORCID and Google buttons on the /account/settings page (which use the old-style frontend stuff). The CSS isn't ORCID-specific so I just renamed it.

TODO: Replace "sso" with "sociallogin" here, for consistency with names used in the Python code.

[seanh]

TODO: Replace "sso" with "sociallogin" here, for consistency with names used in the Python code.

Done: a8609db

[robertknight]

I verified that functionally the whole flow is working by implementing the changes in #9764 on top of this branch.

[robertknight]

I found an issue with the orcid_url generation due to how ORCID_HOST is set in setenv compared to the tests. Otherwise everything worked as expected and I was able to test with the changes in #9764.

[robertknight]

A docstring would be useful to indicate the meaning of these. This could be a class-level docstring that just references the RFC.

[seanh]

Done: bf533db

[robertknight]

IdentityProvider is singular and JWTIssuers is plural, but both are enums. To me it would feel more conventional to always use the singular for an enum (as in "{enum_value} is an {enum_name}")

[seanh]

Done: a1d406f

[robertknight]

Thought for the future: If we have a public URL for the identity, it might be helpful to include that in the configuration for the /signup/orcid view. Then the ID reference in Connected: {ID} could link to it.

[seanh]

👍 Yeah we could do that: https://github.com/orgs/hypothesis/projects/158/views/1?pane=issue&itemId=121932887

[robertknight]

What are the possible values of action here? Just login and connect?

[seanh]

Yes, and actually there's already a literal type for this used elsewhere in the same file, fixed: e6ff9fe

[robertknight]

A docstring would be useful for this field, as the meaning is less obvious than the others.

[seanh]

Done: a5a5014

[robertknight]

This is presumably a route name rather than a route URL?

[seanh]

Yep. Renamed the variable to clarify: 2dd77b1

[robertknight]

This link took me to http://hypothesis.local:5000/account/0000-0002-1961-4147 in testing. This is related to ORCID_HOST being set to a hostname in the dev env, but the tests set it to a URL.

[seanh]

Fixed 327fb1f

[robertknight]

This sets ORCID_HOST to a hostname, but the test_get_returns_orcid_data test sets the value to a URL with a scheme. As a result the orcid_url is generated correctly in tests but doesn't work in the app.

[seanh]

Fixed 327fb1f

[robertknight]

Suggested change

	"""Our OAuth/OIDC client_id for this provider."""
	"""Our OAuth/OIDC client_secret for this provider."""