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

Fixed issue #17654 : spurious error "Incorrect username …" in webserver Auth #2448

Merged
merged 5 commits into from Oct 6, 2022

Conversation

Shnoulle
Copy link
Collaborator

@Shnoulle Shnoulle commented May 31, 2022

Fixed issue #18169: Potential redirect loop with Authwebserver
Dev: Add optional param \LimeSurvey\PluginManager\PluginEvent when potentially needed

…plugin

Dev: Add optionnal param \LimeSurvey\PluginManager\PluginEvent when potentially needed
@Shnoulle
Copy link
Collaborator Author

Update API version … new feature in API : 5.4

@Shnoulle Shnoulle changed the title Fixed issue #spurious error "Incorrect username …" in webserver Auth Fixed issue #17654 : spurious error "Incorrect username …" in webserver Auth Jun 2, 2022
Dev: check if user is allwed to connect in beforeLogin and newUserSession
Dev: throw 401 if webserver is default, allow DB auth else
@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Jun 2, 2022

Error: '/home/runner/work/LimeSurvey/LimeSurvey/third_party/composer/tmp-39795d923d59d80ad603e95578ec9345' is a corrupted zip archive (0 bytes), try again. thi nk it's not related with the commit ;)

Copy link
Collaborator

@gabrieljenik gabrieljenik left a comment

Choose a reason for hiding this comment

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

Code structure looks OK. Haven't tested it

@gabrieljenik gabrieljenik added Code review done Version checked for code issue without testing and removed Needs code review labels Jul 5, 2022
@Shnoulle Shnoulle added Tested OK This PR has been tested by QA and works as expected and removed Needs testing labels Jul 29, 2022
@olleharstedt olleharstedt removed the Code review done Version checked for code issue without testing label Aug 3, 2022
@olleharstedt
Copy link
Contributor

@Shnoulle Did you test your own PR? o0

@Shnoulle
Copy link
Collaborator Author

I check again after last commit,

Else : https://bugs.limesurvey.org/view.php?id=17654#c67723

@olleharstedt
Copy link
Contributor

Really, a developer testing his/her own stuff is not a valid test...

@Shnoulle
Copy link
Collaborator Author

Really, a developer testing his/her own stuff is not a valid test...

prigaux test the previous one #2170
But let's go to have another test with this one

@Shnoulle Shnoulle added Needs testing and removed Tested OK This PR has been tested by QA and works as expected labels Sep 12, 2022
@Shnoulle
Copy link
Collaborator Author

Connect as superadmin (and stay connected)
Create an user TEST for login , just give him right to connect via AuthDB
configure Authwebserver to get user by $_SERVER['TEST_USER'];
update config.php and add $_SERVER['TEST_USER'] = 'TEST'; at start
Open another browser, go to limesurvey admin page

Ad check with removing TEST user with autocreate and without.

https://bugs.limesurvey.org/view.php?id=18169

@brenard
Copy link

brenard commented Sep 19, 2022

Hello, in case of an upgrade if a limesurvey instance using Authserver, I reproduce this bug : my users haven't the auth_webserver permission and have a loop during their authentication.

My diagnostic was complexified because in the admin interface, when I try to edit their permissions, the corresponding permission is checked (even if I can't find it in database). By saving permissions of this user, without changing anything, the permission is effectively added in database and the user is enabled to connect. Do you think it could be also fixed ?

Futhermore, in my case, I didn't find a proper solution to bulk add the missing permission to my users. When trying to fix it, I found this post suggering to add the $config['auth_webserver'] = true; in config.php to make new users to automatically have this permission. This parameter already exists in config-defaults.php and the documentation specify that it is to do, but I thought I'd understood that all parameters have been migrating in the plugin configuration. Moreover, in the plugin code, the permission is automatically added to all users. I think to documentaion and help center isn't uptodate.

To finish, if you have any advice to how to add the missing permission after upgrading, it could be useful for me (and idealy documented in upgrade process).

@Shnoulle
Copy link
Collaborator Author

Hello, in case of an upgrade if a limesurvey instance using Authserver, I reproduce this bug : my users haven't the auth_webserver permission and have a loop during their authentication.

Can you test with this commit ?
This commit must fix this issue …

@brenard
Copy link

brenard commented Sep 20, 2022

Hello, in case of an upgrade if a limesurvey instance using Authserver, I reproduce this bug : my users haven't the auth_webserver permission and have a loop during their authentication.

Can you test with this commit ? This commit must fix this issue …

I just test your commits by patching the two files and it seem working great : existing users without the auth_webserver permission could login. I'm not sure to understand how it's work because, in beforeLogin() method, you seem authorized only existing users with the auth_webserver permission or inexisting user when autocreate is enabled.

@Shnoulle
Copy link
Collaborator Author

Hello, in case of an upgrade if a limesurvey instance using Authserver, I reproduce this bug : my users haven't the auth_webserver permission and have a loop during their authentication.

Can you test with this commit ? This commit must fix this issue …

I just test your commits by patching the two files and it seem working great : existing users without the auth_webserver permission could login. I'm not sure to understand how it's work because, in beforeLogin() method, you seem authorized only existing users with the auth_webserver permission or inexisting user when autocreate is enabled.

Yes it's the case. And if you set AuthWeb by default : throw a clean 401. Else : leave AuthDB doing his own task.

@brenard
Copy link

brenard commented Sep 21, 2022

Yes it's the case. And if you set AuthWeb by default : throw a clean 401. Else : leave AuthDB doing his own task.

OK, so it seem not working as expected unless the auth_webserver permission is defaulty enabled to all users.

In my case, with an user without the auth_webserver permission (check in DB) and AuthWeb set by default, I haven't the expected 401 error.

By adding some debug in your code, I see that the Permission::model()->hasGlobalPermission() check return True, so this suggests that the permission is true by default, but is this the expected behavior? If so, I don't understand the interest of this permission because I don't see how we could disable it.

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Sep 21, 2022

If i don't make error Permission::model()->hasGlobalPermission('auth_webserver', 'read', $oUser->uid) return true for superadmin user.

It's the case here ?

Show the Permission settings for this user.

@brenard
Copy link

brenard commented Sep 21, 2022

If i don't make error Permission::model()->hasGlobalPermission('auth_webserver', 'read', $oUser->uid) return true for superadmin user.

It's the case here ?

You're right ! I try with another user (not superadmin) and I have the 401 error. So it's seem work as expected.

Do you have any advice to how to bulk adding the missing permission to my users ? As said before, it could be great to document a good method in the upgrade process.

@Shnoulle
Copy link
Collaborator Author

Do you have any advice to how to bulk adding the missing permission to my users ? As said before, it could be great to document a good method in the upgrade process.

No … except db update via direct SQL

@Shnoulle
Copy link
Collaborator Author

@olleharstedt i remove the need testing part …

@Shnoulle Shnoulle added Tested OK This PR has been tested by QA and works as expected and removed Needs testing labels Sep 21, 2022
@olleharstedt
Copy link
Contributor

Should be merged to develop?

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Oct 4, 2022

Should be merged to develop?

There are real issue currently with WebAuth … with or without WebAuth set as default.

This one for example : https://bugs.limesurvey.org/view.php?id=18169 if user is set by SERVER : always get a loop (if it don't have the right to connect to AuthWeb)

@olleharstedt olleharstedt merged commit 7066f69 into LimeSurvey:master Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Technical Technical update / enhancement Tested OK This PR has been tested by QA and works as expected
Projects
None yet
4 participants