Skip to content

Decouple auth from http #468

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Decouple auth from http #468

wants to merge 1 commit into from

Conversation

JordonPhillips
Copy link
Contributor

@JordonPhillips JordonPhillips commented Mar 26, 2025

This decouples auth from HTTP, allowing it to be defined centrally.

A number of changes have been made to various interfaces. Notably identity resolvers are generally expected to have zero-arg constructors, instead getting everything they need from their properties where possible.

Construction of event signers has been moved into AuthScheme.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@JordonPhillips JordonPhillips force-pushed the no-http-auth branch 2 times, most recently from 4356949 to fb5bf53 Compare April 8, 2025 14:26
@JordonPhillips JordonPhillips marked this pull request as ready for review April 8, 2025 14:32
@JordonPhillips JordonPhillips requested a review from a team as a code owner April 8, 2025 14:32
Copy link
Contributor

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

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

Need to do one more pass tomorrow, but looks good. Just one question on why we're making some of the types private.

@JordonPhillips JordonPhillips changed the title [DRAFT] Decouple auth from http Decouple auth from http Apr 11, 2025
@JordonPhillips JordonPhillips force-pushed the no-http-auth branch 2 times, most recently from dbae11e to 6dbca91 Compare May 2, 2025 15:06

Signers are constructed by the `AuthScheme`'s `signer` method.

Signers MAY modify the given request and return it, or construct a new signed
Copy link
Contributor

Choose a reason for hiding this comment

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

Signers MAY modify the given request and return it

If we're recommending this, do we know how it's going to interact with retries for generic clients? I think that's the primary concern prompting creating a new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already will create a copy of the request before each attempt, so the potential ramifications are limited.

expiration: datetime | None = None
"""The expiration time of the identity.

If time zone is provided, it is updated to UTC. The value must always be in UTC.
Copy link
Contributor

Choose a reason for hiding this comment

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

We say this in a bunch of places but I'm not sure I've seen an implementation where we actually do this. Is this a note to implementers, or should we have some base implementation that checks timezones and makes adjustments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The identity resolver is expected to do that. I could update this to a dataclass and drop it in post_init?

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems reasonable if we have a base implementation. Otherwise, it may be worth simplifying the wording to just "All date times with a timezone must be in UTC."

@JordonPhillips JordonPhillips requested a review from nateprewitt May 7, 2025 10:40
This decouples auth from HTTP, allowing it to be defined centrally.

A number of changes have been made to various interfaces. Notably
identity resolvers are generally expected to have zero-arg
constructors, instead getting everything they need from their
properties where possible.

Construction of event signers has been moved into AuthScheme. Right
now it takes the entire context of the request during construction,
but we should consider passing identity and signing properties in
at signing time like normal signers so that they can be reused.
expiration: datetime | None = None
"""The expiration time of the identity.

If time zone is provided, it is updated to UTC. The value must always be in UTC.
Copy link
Contributor

Choose a reason for hiding this comment

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

That seems reasonable if we have a base implementation. Otherwise, it may be worth simplifying the wording to just "All date times with a timezone must be in UTC."

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.

2 participants