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

Logout existing sessions after an auth config change #21304

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Func86
Copy link

@Func86 Func86 commented Sep 6, 2024

Closes #18443.

@glassez glassez added the WebAPI WebAPI-related issues/changes label Sep 15, 2024
@glassez glassez requested a review from a team September 15, 2024 12:05
src/webui/api/authcontroller.cpp Outdated Show resolved Hide resolved
src/webui/api/authcontroller.cpp Outdated Show resolved Hide resolved
@Func86 Func86 force-pushed the session-reset branch 2 times, most recently from f0ccfa3 to c896268 Compare September 15, 2024 12:16
@Func86 Func86 requested a review from glassez September 15, 2024 12:21
@glassez glassez requested a review from a team September 17, 2024 02:40
@glassez glassez added this to the 5.1 milestone Sep 17, 2024
Copy link
Member

@Chocobo1 Chocobo1 left a comment

Choose a reason for hiding this comment

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

@Func86
I think the current state of the PR is the wrong approach.

You should invoke logoutAllSessions() in if (hasKey(u"web_ui_username"_s)) and if (hasKey(u"web_ui_password"_s)) in appcontroller.cpp. And in if (const QString username = webUIUsername(); isValidWebUIUsername(username)) & if (const QString password = webUIPassword(); isValidWebUIPassword(password)) in optionsdialog.cpp
And you should not bother to check the previous value, just invoke logoutAllSessions() within the if block.

UPDATE, disregard the above. It should be done in WebUI::configure() instead. This is where the server updates the configuration.

@glassez
Copy link
Member

glassez commented Sep 17, 2024

UPDATE, disregard the above. It should be done in WebUI::configure() instead. This is where the server updates the configuration.

The same as currently. WebUI will have to store the previous username/password values to detect that they have changed.

@Chocobo1
Copy link
Member

Chocobo1 commented Sep 17, 2024

The same as currently. WebUI will have to store the previous username/password values to detect that they have changed.

I think the following should work:

  1. add a new signal Preferences::webCredentialsChanged()
  2. the new signal will be emitted when the webui username updated or password hash updated. (no need to compare to the old value, just emit the signal)
  3. WebApplication will connect to the signal and remove all sessions when receiving the signal.

@glassez
Copy link
Member

glassez commented Sep 17, 2024

I think the following should work:

  1. add a new signal Preferences::webCredentialsChanged()
  2. the new signal will be emitted when the webui username updated or password hash updated. (no need to compare to the old value, just emit the signal)
  3. WebApplication will connect to the signal and remove all sessions when receiving the signal.

It could do the job.

Closes #18443.

It also mentions some other settings besides the username and password, changing which should lead to the termination of sessions.

How about it?

@Func86 Func86 marked this pull request as draft September 17, 2024 14:59
@Func86 Func86 force-pushed the session-reset branch 2 times, most recently from 33ac9d1 to 12e1112 Compare September 17, 2024 16:28
@Func86 Func86 marked this pull request as ready for review September 17, 2024 16:29
@Func86
Copy link
Author

Func86 commented Sep 17, 2024

It also mentions some other settings besides the username and password, changing which should lead to the termination of sessions.

How about it?

Done.

@Func86 Func86 changed the title Logout existing sessions after a username or password change Logout existing sessions after an auth config change Sep 17, 2024
@Func86 Func86 force-pushed the session-reset branch 3 times, most recently from 4050ed9 to 8edda6b Compare September 17, 2024 18:50
src/base/preferences.cpp Outdated Show resolved Hide resolved
src/base/preferences.cpp Outdated Show resolved Hide resolved
Comment on lines 1984 to 1985
if (m_credentialsChanged)
emit webCredentialsChanged();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (m_credentialsChanged)
emit webCredentialsChanged();
if (m_credentialsChanged)
{
emit webCredentialsChanged();
m_credentialsChanged = false;
}

Copy link
Member

Choose a reason for hiding this comment

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

Do you still need the changes in isessionmanager.h after applying the above suggestions?

Copy link
Author

Choose a reason for hiding this comment

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

Do you still need the changes in isessionmanager.h after applying the above suggestions?

What? The change in isessionmanager.h is for terminating sessions which bypassed authentication when the bypass is disabled or changed to possibly stricter than before.

Copy link
Member

Choose a reason for hiding this comment

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

The change in isessionmanager.h is for terminating sessions which bypassed authentication when the bypass is disabled or changed to possibly stricter than before.

Why not just remove all sessions unconditionally when the option changed?

Copy link
Author

Choose a reason for hiding this comment

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

I think it would be surprising for users with authenticated sessions to find they are logged out after changing these options.

By the way, we may want to disallow unauthenticated sessions to change the username/password.

src/webui/webapplication.h Outdated Show resolved Hide resolved
src/webui/webapplication.cpp Outdated Show resolved Hide resolved
src/webui/webapplication.cpp Outdated Show resolved Hide resolved
src/webui/webapplication.cpp Outdated Show resolved Hide resolved
src/webui/webapplication.cpp Outdated Show resolved Hide resolved
Comment on lines 1984 to 1985
if (m_credentialsChanged)
emit webCredentialsChanged();
Copy link
Member

Choose a reason for hiding this comment

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

The change in isessionmanager.h is for terminating sessions which bypassed authentication when the bypass is disabled or changed to possibly stricter than before.

Why not just remove all sessions unconditionally when the option changed?

@Func86 Func86 force-pushed the session-reset branch 2 times, most recently from 68d3b4c to aad811a Compare September 20, 2024 15:45
@Func86 Func86 force-pushed the session-reset branch 2 times, most recently from c904cd0 to 1782c3b Compare September 20, 2024 15:48
src/webui/webapplication.h Outdated Show resolved Hide resolved
src/webui/webapplication.h Outdated Show resolved Hide resolved
src/webui/webapplication.h Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebAPI WebAPI-related issues/changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WebUI Security: Session still valid after reset password but before restart qBittorrent
3 participants