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.

Sign up for GitHub

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

Jump to bottom

Logout via Query Params #3

Closed
wants to merge 2 commits into from
Closed

Logout via Query Params #3

wants to merge 2 commits into from

Conversation

bwaidelich
Copy link
Collaborator

@bwaidelich bwaidelich commented Jul 30, 2019 •
edited
Loading

  • Remove "cookieName" configuration and assume "<serviceName>-jwt" as
    cookie name
  • Adjust SetJwtCookieComponent to handle logout (remove JWT cookie
    and trigger redirect) if ?logout=<serviceName> query parameter is
    present

Closes: #2

@bwaidelich
Logout via Query Params
35d1eeb 
* Remove "cookieName" configuration and assume "<serviceName>-jwt" as
  cookie name
* Adjust `SetJwtCookieComponent` to handle logout (remove JWT cookie
  and trigger redirect) if `?logout=<serviceName>` query parameter is
  present

Closes: flownative#2
@kitsunet
Copy link
Member

That all looks pretty reasonable from my side and probably ready to merge, BUT why remove the cookieName configuration? Can't it be $configuredCookieName . '-serviceName' ?

bwaidelich
bwaidelich commented Jul 31, 2019
View reviewed changes
Classes/Http/SetJwtCookieComponent.php
{
$this->removeJwtCookie($componentContext);
$this->logger->info(sprintf('OpenID Connect Client: (%s) Logout requested (via query parameter) removing JWT cookie for service "%s".', get_class($this), $this->options['serviceName']));
$componentContext->replaceHttpResponse($componentContext->getHttpResponse()->withHeader('Location', (string)$componentContext->getHttpRequest()->getUri()->withQuery('')));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This part is a bit magic.. It redirects to the current URI without query params. If other params are required they would be missing. I think we have to revise this part, but without the redirect the account is already authenticated

Copy link
Member

Choose a reason for hiding this comment

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

What about a defined "afterLogout" URL?

@bwaidelich
Copy link
Collaborator Author

bwaidelich commented Jul 31, 2019 •
edited
Loading

Thanks for the review!

why remove the cookieName configuration?

This package already has the notion of a "serviceName" to make it multi-tenant capable. Instead of duplicating the configuration everywhere we thought it would be better to "centralize" the oAuth tenants via the "serviceName".
But you're right, this is a breaking change and handling the logout in the SetCookieComponent is a bit awkward anyways..

I'll solve this in our application layer for now, until we found a better generic approach

@codecov
Copy link

codecov bot commented Dec 17, 2020

Codecov Report

Merging #3 (3ea2216) into master (9399467) will decrease coverage by 0.49%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master       #3      +/-   ##
============================================
- Coverage     15.38%   14.88%   -0.50%     
- Complexity      189      194       +5     
============================================
  Files             9        9              
  Lines           481      497      +16     
============================================
  Hits             74       74              
- Misses          407      423      +16
Impacted Files Coverage Δ Complexity Δ
Classes/Authentication/OpenIdConnectProvider.php 0.00% <0.00%> (ø) 33.00 <0.00> (-1.00)
Classes/Http/SetJwtCookieComponent.php 0.00% <0.00%> (ø) 21.00 <5.00> (+6.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9399467...3ea2216. Read the comment docs.

@kdambekalns
Copy link
Member

Gah, I guess conflict resolution via GH is a bad idea for all but the simples cases. Feel free to throw away 3ea2216 again. Anyway, this touches code that has changed a bit in the meantime.

@bwaidelich
Copy link
Collaborator Author

The suggested solution was breaking and I'm not really happy with it.. Closing for now to remove this from my pending PRs

@bwaidelich bwaidelich closed this Nov 1, 2023
robertlemke pushed a commit that referenced this pull request Nov 29, 2023
@christoph-daehne
Merge pull request #3 from flownative/main
3df14d1 
Importing fixes from base repository
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kitsunet kitsunet kitsunet left review comments

@robertlemke robertlemke Awaiting requested review from robertlemke

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Logout via Query Parameter
3 participants
@bwaidelich @kitsunet @kdambekalns