Skip to content

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Dec 18, 2021

When logging in the SessionID should be reset and the session cleaned up.

Also logs the user in on completion of linking account

Signed-off-by: Andrew Thornton art27@cantab.net

@zeripath zeripath added this to the 1.16.0 milestone Dec 18, 2021
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 18, 2021
@zeripath zeripath force-pushed the reset-session-id-on-login branch from c222614 to 5e44779 Compare December 18, 2021 13:24
When logging in the SessionID should be reset and the session cleaned up.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath force-pushed the reset-session-id-on-login branch from 5e44779 to a63ce34 Compare December 18, 2021 14:53
@zeripath

This comment has been minimized.

@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@e4e4118). Click here to learn what that means.
The diff coverage is 12.96%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #18018   +/-   ##
=======================================
  Coverage        ?   45.17%           
=======================================
  Files           ?      824           
  Lines           ?    91436           
  Branches        ?        0           
=======================================
  Hits            ?    41305           
  Misses          ?    43536           
  Partials        ?     6595           
Impacted Files Coverage Δ
routers/web/user/auth_openid.go 0.00% <0.00%> (ø)
services/auth/auth.go 27.11% <0.00%> (ø)
services/auth/source/oauth2/store.go 0.00% <0.00%> (ø)
routers/web/user/auth.go 11.14% <10.52%> (ø)
modules/session/store.go 100.00% <100.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 e4e4118...2f85e32. Read the comment docs.

@zeripath

This comment has been minimized.

Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
… account linking pages too

Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 20, 2021
@lunny
Copy link
Member

lunny commented Dec 20, 2021

I found if user enabled 2fa, the sessionid changed when password checked. But when second verify checked, session id will not changed.
Is that by design?

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 20, 2021
@lunny lunny merged commit bcc13f3 into go-gitea:main Dec 20, 2021
zeripath added a commit to zeripath/gitea that referenced this pull request Dec 20, 2021
* Reset Session ID on login

When logging in the SessionID should be reset and the session cleaned up.

Signed-off-by: Andrew Thornton <art27@cantab.net>

* with new session.RegenerateID function

Signed-off-by: Andrew Thornton <art27@cantab.net>

* update go-chi/session

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Ensure that session id is changed after oauth data is set and between account linking pages too

Signed-off-by: Andrew Thornton <art27@cantab.net>

* placate lint

Signed-off-by: Andrew Thornton <art27@cantab.net>

* as per review

Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath added a commit that referenced this pull request Dec 20, 2021
Backport #18018

When logging in the SessionID should be reset and the session cleaned up.

Also logs the user in on completion of linking account

Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath added a commit to zeripath/gitea that referenced this pull request Dec 20, 2021
Add:

* Move POST /{username}/action/{action} to simply POST /{username} (go-gitea#18045) (go-gitea#18046)
* Fix delete u2f keys bug (go-gitea#18040) (go-gitea#18042)
* Reset Session ID on login (go-gitea#18018) (go-gitea#18041)
* Prevent off-by-one error on comments on newly appended lines (go-gitea#18029) (go-gitea#18035)

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this pull request Dec 20, 2021
@zeripath zeripath added the topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! label Dec 22, 2021
@zeripath zeripath deleted the reset-session-id-on-login branch December 22, 2021 09:12
@zeripath zeripath added the backport/done All backports for this PR have been created label Dec 25, 2021
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
* Reset Session ID on login

When logging in the SessionID should be reset and the session cleaned up.

Signed-off-by: Andrew Thornton <art27@cantab.net>

* with new session.RegenerateID function

Signed-off-by: Andrew Thornton <art27@cantab.net>

* update go-chi/session

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Ensure that session id is changed after oauth data is set and between account linking pages too

Signed-off-by: Andrew Thornton <art27@cantab.net>

* placate lint

Signed-off-by: Andrew Thornton <art27@cantab.net>

* as per review

Signed-off-by: Andrew Thornton <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants