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

3ev/feature/734 cognito ammends #111

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PaulB12
Copy link

@PaulB12 PaulB12 commented Nov 15, 2024

Requested Change 1 - Two new config vars: cognito.endpoint & cognito.scheme

This PR wishes to add two new config variables called:
'endpoint' => config('cognito.endpoint'),
'scheme' => config('cognito.scheme')

The reason for this requested change is that we wish to unit test without having to use a live AWS cognito pool, instead we are able to use a local docker container that mocks AWS cognito pool however it'll require changing the endpoint.

This change does not break backwards compatability.

Requested Change 2 - Refactor of refreshToken

It seems that the refreshToken trait relies on fetching the user via a access token being set in the Auth header or session, however an accessToken can be expired but a refreshToken still valid.

Instead this refactor allows the passing in of a $user model to fetch the sub Id, it will default back to the original way if not set.

In addition the refreshToken wasn't being invalidated after being used, the token is now invalidated.

@amitdhongde
Copy link
Collaborator

amitdhongde commented Jan 1, 2025

Hi @PaulB12 , Thank you for the PR. I shall surely review the changes and merge.

For the Refresh Token, Are you suggesting the change in TokenGaurd? Calling a refresh token requires an authenticated endpoint, and without an Access Token, calling the refresh endpoint does not make sense however, the use case you mentioned is valid for Web Sessions only.

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