Skip to content

Conversation

woodruffw
Copy link
Member

@woodruffw woodruffw commented May 3, 2023

WIP.

This also needs some more changes, but I'll wait for #13566 to make them.

See #13551.

@woodruffw woodruffw marked this pull request as ready for review May 3, 2023 18:14
@woodruffw woodruffw requested a review from a team as a code owner May 3, 2023 18:14
@woodruffw
Copy link
Member Author

Needs tests and other work, but the basic structure here is good for an initial review!

woodruffw added 2 commits May 3, 2023 14:25
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
@woodruffw woodruffw requested a review from di May 3, 2023 19:03
@woodruffw woodruffw self-assigned this May 3, 2023
@di
Copy link
Member

di commented May 3, 2023

This is getting a little bit unwieldy and duplicative -- given that we're likely going to add a third issuer soon, I think we should find a way to simplify this.

One thought is that we could a) have a lookup table that maps from (issuer, pending) -> OIDCPublisher b) store the filter expressions on the respective OIDCPublisher instead. So the function would be something like:

publisher_classes = {
    GITHUB_OIDC_ISSUER_URL: [GitHubPublisher, PendingGitHubPublisher],
    GOOGLE_OIDC_ISSUER_URL: [GooglePublisher, PendingGooglePublisher],
}

def find_publisher_by_issuer(session, issuer_url, signed_claims, *, pending=False):
    try:    
        publisher_cls = publisher_classes[issuer_url][pending]
    except KeyError:
        return None
        
    # Check for a publisher with options
    publisher =  (
        session.query(publisher_cls)
        .filter_by(publisher_cls.filter_expr_with_options(signed_claims))
        .one_or_none()
    )
    # Check for a fallback publisher
    return (
        session.query(publisher_cls)
        .filter_by(publisher_cls.filter_expr_without_options(signed_claims))
        .one_or_none()
    )

@woodruffw
Copy link
Member Author

That sounds good to me! The class mapping part is certainly doable; storing the expressions might be slightly trickier (since e.g. the GitHub one contains an expression that gets built from a string match).

Do you want me to look into that with this PR, or hold off until the next round of publishers (e.g. GitLab)?

@di
Copy link
Member

di commented May 3, 2023

Do you want me to look into that with this PR, or hold off until the next round of publishers (e.g. GitLab)?

I think I'd like to try to do it here, maybe take a crack at it and see how much of a time suck it'll be? Fine with me if you think it's not worth the effort though!

@woodruffw
Copy link
Member Author

I think I'd like to try to do it here, maybe take a crack at it and see how much of a time suck it'll be?

Let's find out 🙂

@woodruffw
Copy link
Member Author

woodruffw commented May 3, 2023

One thought: how should this work if/when we have publishers with multiple optional claims? It should be fine if we don't include multiple optionals in the unique constraint set, but if we do it'll probably result in N! stored expressions for N possible optional claims.

This doesn't solve that problem, but in terms of devolving it:

for lookup in publisher_cls.lookups:
    if publisher := session.query(publisher_cls).filter(lookup(signed_claims)):
        return publisher

(Where lookup is a bad name, but would be each of these stored filter expressions.)

@di
Copy link
Member

di commented May 3, 2023

Maybe premature optimization? But I think a list of N!+1 increasingly loosened filter expressions would also satisfy the current two-query case as well, so might be worth doing. The alternative would be writing N!+1 queries, so I don't think there's any way around the general behavior here.

@woodruffw
Copy link
Member Author

Actually, thinking about it some more: these don't even need to be written out manually (which would be error-prone), since they're expressible via the existing __required_verifiable_claims__ and __optional_verifiable_claims__ on each publisher, whether pending or not!

I'll play around with that a bit 🙂

This devolves the lookup logic for each publisher type back
to its actual class (or base mixin, as appropriate).

The downside is that it's a little magical: each OIDCPublisher
now provides __lookup_strategies__, which is a list of callables
that resolve a model type and a claim set into an optional query,
which can then be realized with a DB session into exactly one
or none publishers.

Signed-off-by: William Woodruff <[email protected]>

tests: update tests

Signed-off-by: William Woodruff <[email protected]>
@woodruffw
Copy link
Member Author

woodruffw commented May 3, 2023

059b1e6 is my attempt at this: the lookup logic is now devolved back to each publisher (or more accurately, the "base" mixin between pending and non-pending publishers of each type).

Each publisher type now supplies __lookup_strategies__, which provides a list of callables that return queries.

A bit of nastiness happens with these callables: conceptually they're @classmethods, but class methods aren't actually callable (because they're indeterminate until their surrounding class is finalized), so they're actually static methods that happen to take a class object as their first argument. I've declared them inside their relevant classes for the sake of locality, but this could be resolved by putting them at the module scope instead.

@woodruffw
Copy link
Member Author

This should be good to go again.

@di di merged commit 2cabb23 into pypi:main Jun 5, 2023
@di di deleted the tob-google-oidc-services branch June 5, 2023 21:47
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.

2 participants