fix(authn): create sessions only if UI header value is supplied #1919
Conversation
eusebiu-constantin-petu-dbk
requested review from
rchincha and
andaaron
as code owners
Codecov Report
@@ Coverage Diff @@
## main #1919 +/- ##
==========================================
- Coverage 91.94% 91.93% -0.01%
==========================================
Files 153 153
Lines 26437 26440 +3
==========================================
+ Hits 24307 24308 +1
- Misses 1573 1574 +1
- Partials 557 558 +1
... and 5 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| if err := saveUserLoggedSession(cookieStore, response, request, identity, ctlr.Log); err != nil { | ||
| return false, err | ||
| // saved logged session only if the request comes from web (has UI session header value) | ||
| if hasSessionHeader(request) { |
There was a problem hiding this comment.
do we need to take care of deleting these sessions though?
There was a problem hiding this comment.
using the current library, no... I can not get a reference for an expired session.
In case of an expired session, the library will return an error (expired session) and a new valid session. losing the ability to get the reference of the expired session and delete it.
There was a problem hiding this comment.
there are a lot of issues about this:
gorilla/sessions#160
they say how can you delete, but they don't say how to delete expired session, in another issue they state that expired sessions are overwritten by new sessions, but that's not true, at least for our case.
There was a problem hiding this comment.
IMHO let's fix what we can be fixed with this library, and track relacing it with something else (another sessions library? or tokens? we need a discussion) separately.
eusebiu-constantin-petu-dbk
force-pushed
the
fix_web_sessions
branch
from
cd3d85a to
fe4841c
Compare
Signed-off-by: Petu Eusebiu <[email protected]>
eusebiu-constantin-petu-dbk
force-pushed
the
fix_web_sessions
branch
from
fe4841c to
079dbaf
Compare
What type of PR is this?
bug
Which issue does this PR fix:
#1910
What does this PR do / Why do we need it:
If an issue # is not available please add repro steps and logs showing the issue:
Testing done on this change:
Automation added to e2e:
Will this break upgrades or downgrades?
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.