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

chore: Refactor Auth Token Middleware's token logic #492

Merged
merged 10 commits into from
Nov 15, 2023

Conversation

yash30201
Copy link
Contributor

@yash30201 yash30201 commented Oct 26, 2023

Delegate Auth Metadata logic to Auth Library

Implement the go/php-auth-token-unifomity design to delegate token fetching and metadata updation logic to the auth library, resulting transfer of auth metadata control to auth library, thus unifying the token flow.

Post this change, all the php api client services libraries would adhere to go/php-auth-token-unifomity.

Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

The way you have it now seems to be making more changes than necessary. If the goal of this PR is to just to use updateMetadata when available, I think it would be better to simplify this logic as something like this:

// Use "updateMetadata" if it exists, otherwise use "fetchAuthToken"
if (!$this->fetcher instanceof UpdateMetadataInterface ||
   ($this->fetcher FetchAuthTokenCache && !$this->fetcher->getFetcher() instanceof UpdateMetadataInterface)
) {
    // previous behavior (unlikely - only happens if user supplies a custom fetcher)
    $request = $request->withHeader('authorization', 'Bearer ' . $this->fetchToken());
} else {
    // new behavior - allow the credentials to add / update request headers
    $updatedHeaders = $this->fetcher->updateMetadata($request->getHeaders(), null, $this->httpHandler);
    $request = Utils::modifyRequest($request, ['set_headers' => $updatedHeaders]);
}

src/Middleware/AuthTokenMiddleware.php Outdated Show resolved Hide resolved
@yash30201
Copy link
Contributor Author

yash30201 commented Oct 31, 2023

The way you have it now seems to be making more changes than necessary. If the goal of this PR is to just to use updateMetadata when available, I think it would be better to simplify this logic as something like this:

// Use "updateMetadata" if it exists, otherwise use "fetchAuthToken"
if (!$this->fetcher instanceof UpdateMetadataInterface ||
   ($this->fetcher FetchAuthTokenCache && !$this->fetcher->getFetcher() instanceof UpdateMetadataInterface)
) {
    // previous behavior (unlikely - only happens if user supplies a custom fetcher)
    $request = $request->withHeader('authorization', 'Bearer ' . $this->fetchToken());
} else {
    // new behavior - allow the credentials to add / update request headers
    $updatedHeaders = $this->fetcher->updateMetadata($request->getHeaders(), null, $this->httpHandler);
    $request = Utils::modifyRequest($request, ['set_headers' => $updatedHeaders]);
}

Thanks for the pointer. We would be required to fetch auth token when using update metadata because we need to call the token callback in the event it isn't an identity token request. I've amended the logic as such, please take a look. Now it looks cleaner.

@yash30201 yash30201 marked this pull request as ready for review November 2, 2023 13:39
@yash30201 yash30201 requested a review from a team as a code owner November 2, 2023 13:39
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

Good stuff! We're getting closer here.

src/Middleware/AuthTokenMiddleware.php Outdated Show resolved Hide resolved
src/Middleware/AuthTokenMiddleware.php Show resolved Hide resolved
src/Middleware/AuthTokenMiddleware.php Outdated Show resolved Hide resolved
yash30201 and others added 2 commits November 3, 2023 10:00
Co-authored-by: Brent Shaffer <[email protected]>
1 ) Refactored authMiddleware logic to make it look clean.
2 ) Fixed tests according to the change
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

This is looking great! I only have some suggestions on how to improve the tests.

tests/FetchAuthTokenTest.php Show resolved Hide resolved
tests/FetchAuthTokenTest.php Outdated Show resolved Hide resolved
tests/Middleware/AuthTokenMiddlewareTest.php Outdated Show resolved Hide resolved
tests/Middleware/AuthTokenMiddlewareTest.php Show resolved Hide resolved
tests/Middleware/AuthTokenMiddlewareTest.php Show resolved Hide resolved
tests/Middleware/AuthTokenMiddlewareTest.php Outdated Show resolved Hide resolved
1) Fix indentation
2) Simplify `runTestCase` method in AuthTokenMiddlewareTest
3) Improve testing.
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