Added logout functionality #60
Conversation
|
Hi Artur, thanks for creating this pull request. I will do my best to review it in the next couple of days. |
1 similar comment
|
Hi Artur, thanks for creating this pull request. I will do my best to review it in the next couple of days. |
There was a problem hiding this comment.
Hi aszniak,
First of all, thank you for your pull request. We really appreciate your contribution to our project!
I apologize for the delay in reviewing your PR. We've been swamped lately and haven't been able to keep up with our usual review timeline.
I've had a chance to review your changes and I think they look great. However, I do have a few comments and suggestions that I think would improve this feature.
| @@ -213,6 +220,41 @@ export class Authenticator { | |||
| const cfDomain = request.headers.host[0].value; | |||
| const redirectURI = `https://${cfDomain}`; | |||
|
|
|||
| try { | |||
| if (this._enableLogout && request.uri === "/logout") { | |||
There was a problem hiding this comment.
It'd be better to make logout URI configurable instead of hardcoding it to "/logout"
| }; | ||
|
|
||
| const expireCookies = true; | ||
| const logoutUrl = `https://${this._userPoolDomain}/logout?logout_uri=${redirectURI}&client_id=${this._userPoolAppId}`; |
There was a problem hiding this comment.
In addition to making the logout URI configurable, it's also important to make the logout redirect URI configurable. This is because the logout redirect URI needs to exactly match the pool client configuration, and allowing users to customize it would ensure that it's set correctly for their specific environment.
|
|
||
| const expireCookies = true; | ||
| const logoutUrl = `https://${this._userPoolDomain}/logout?logout_uri=${redirectURI}&client_id=${this._userPoolAppId}`; | ||
| return this._getRedirectResponse(tokens, cfDomain, logoutUrl, expireCookies); |
There was a problem hiding this comment.
When cookies are cleaned up after a user logs out, the tokens contained within them are still valid. This is especially concerning for the refresh token, which is valid for 30 days by default. I would suggest at least revoking the refresh token.
| @@ -30,6 +31,7 @@ export class Authenticator { | |||
| _disableCookieDomain: boolean; | |||
| _httpOnly: boolean; | |||
| _sameSite?: SameSite; | |||
| _enableLogout?: boolean; | |||
There was a problem hiding this comment.
I would recommend implementing a dedicated LogoutConfiguration. If it is not defined, the logout functionality would be disabled.
interface LogoutConfiguration {
logoutUri: string;
logoutRedirectUri: string;
}
|
Closing as this is redundant with #68 which I just merged. |
*Issue # (if available):
#8
Description of changes:
Creates a behavior that allows to expire cookies and redirect to Cognito /logout endpoint.
If there are no cookies present when trying to reach the logout endpoint, redirects to domain (app) root.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.