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

Fix potential login -> logout loop when allow_multiple_user_backends is 0 #761

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

julien-nc
Copy link
Member

@julien-nc julien-nc commented Jan 16, 2024

This makes sure the user_oidc login page does not allow the redirect_url GET param to be the logout page or /apps/user_oidc/sls (single logout) when allow_multiple_user_backends == 0.

The issue is well described in #743. I could reproduce it with those system setting values:

'auto_logout' => true,
'session_lifetime' => 30,
'session_keepalive' => false,
'remember_login_cookie_lifetime' => 35,

Then authenticate via user_oidc, then wait for the auto logout.

@isdnfan Could you check if it makes things better on your side?

@julien-nc julien-nc added the bug Something isn't working label Jan 16, 2024
@julien-nc julien-nc requested a review from juliusknorr January 16, 2024 10:51
@julien-nc julien-nc changed the title Fix login -> logout loop if allow_multiple_user_backends is 0 Fix potential login -> logout loop when allow_multiple_user_backends is 0 Jan 16, 2024
@julien-nc
Copy link
Member Author

@isdnfan Answering to #743 (comment) here if you don't mind 😁 .

Could you try again? There was a bug in the fix 🙈 . It's now fixed.

The auto redirection didn't work when the redirect_url GET param of the login page was not set. Big side effect: the crash prevented the "alternative login" button to be registered so even when reaching the login page (after the failed redirect), there was no button to manually go to the IdP. All should be fine now.

@isdnfan
Copy link

isdnfan commented Jan 17, 2024

unfortunately the loop is back with he new version:

image

@julien-nc
Copy link
Member Author

The loop that is addressed here is the one started when auto_logout is true and user_oidc's allow_multiple_user_backends is 0.
With those config values:

'auto_logout' => true,
'session_lifetime' => 30,
'session_keepalive' => false,
'remember_login_cookie_lifetime' => 35,

image
When the user is auto-logged out, there's a redirection to login?redirect_url=/dev/server/index.php/logout?requesttoken%3DCY8Cx14ys0exU... which was leading to a loop but now it's redirected to /apps/user_oidc/login/3?redirectUrl=https://nextcloud.local/dev/server. The redirectUrl GET param of the user_oidc login cannot be the logout page anymore.

I tried setting the config values you mentioned in the issue:

'auto_logout' => false,
'session_keepalive' => true,
'session_lifetime' => 30,
'session_relaxed_expiry' => false,
'remember_login_cookie_lifetime' => 0,

but could not enter any loop (with or without the changes). Could you give a precise description of what you do to enter the loop? I need all the steps to be able to reproduce. Also, might not be relevant but: Are you using a chromium based browser?
According to your screenshot, I don't understand how the loop is started because the logout redirect seems to be correctly replaced by your instance base URL
image

@isdnfan
Copy link

isdnfan commented Jan 17, 2024

hi Julien I'm using Firefox 121.0.1 on Windows 10 (but loop happens on Edge as well). I'm doing "nothing" to enter the loop. In my installation deck app is disabled so main app is files

  • I just visit my main page 'https://dev-nc.mydomain.tld'
  • redirect to IdP happens
  • after login I see "Files" app
  • in dev tools every 5 sec there is push/sync message send back and forth
  • after session timeout at 30-35 sec I see
  • GET /logout?requesttoken=...

I was under impression the issue is related to 'auto_logout' after your last update it worked when the setting was true and looped at false. But switching it back and was not really stable. Additionally I observer an interesting behavior: with the last fix the loop happens but after looping for 10 or 20 times there successful login happens and the browser hits the Files app page..

For me 'GET /login?redirect_url=/logout?requesttoken' sounds strange - if I get it right this makes the client login just to proceed with logout again? this request is the first request when the client comes back from IdP. I added the whole browser HAR file - dev-nc. is Nextcloud 28.0.1 and sso. is IdP (Zitadel) - just replaced my domain with <mydomain.tld>.

dev-nc.mydomain.tld_Archive [24-01-17 13-22-52].har.zip

@julien-nc
Copy link
Member Author

@isdnfan Thanks. I need more details to make sure i'm in the same context:

  • config.php values
  • what's the target URL of the push/sync requests?
  • what exactly do you see when the loop starts? Is the logout request done by a script or is the entire page redirected to /logout?

@isdnfan
Copy link

isdnfan commented Jan 18, 2024

hi @julien-nc likely you missed it - I added the HAR file on my last comment - you can inspect all messages with all details there.. you can load the HAR using firefox dev tools

image

from the file I attached:

image

occ config:list system

{
    "system": {
        "htaccess.RewriteBase": "\/",
        "memcache.local": "\\OC\\Memcache\\APCu",
        "apps_paths": [
            {
                "path": "\/var\/www\/html\/apps",
                "url": "\/apps",
                "writable": false
            },
            {
                "path": "\/var\/www\/html\/custom_apps",
                "url": "\/custom_apps",
                "writable": true
            }
        ],
        "overwritehost": "dev-nc.mydomain.tld",
        "overwriteprotocol": "https",
        "passwordsalt": "***REMOVED SENSITIVE VALUE***",
        "secret": "***REMOVED SENSITIVE VALUE***",
        "trusted_domains": [
            "localhost"
        ],
        "datadirectory": "***REMOVED SENSITIVE VALUE***",
        "dbtype": "mysql",
        "version": "28.0.1.1",
        "dbname": "***REMOVED SENSITIVE VALUE***",
        "dbhost": "***REMOVED SENSITIVE VALUE***",
        "dbport": "",
        "dbtableprefix": "oc_",
        "mysql.utf8mb4": true,
        "dbuser": "***REMOVED SENSITIVE VALUE***",
        "dbpassword": "***REMOVED SENSITIVE VALUE***",
        "installed": true,
        "instanceid": "***REMOVED SENSITIVE VALUE***",
        "loglevel": "1",
        "maintenance": false,
        "memcache.distributed": "\\OC\\Memcache\\Redis",
        "memcache.locking": "\\OC\\Memcache\\Redis",
        "redis": {
            "host": "***REMOVED SENSITIVE VALUE***",
            "password": "***REMOVED SENSITIVE VALUE***",
            "port": 6379
        },
        "default_phone_region": "CH",
        "mail_from_address": "***REMOVED SENSITIVE VALUE***",
        "mail_smtpmode": "smtp",
        "mail_sendmailmode": "smtp",
        "mail_domain": "***REMOVED SENSITIVE VALUE***",
        "mail_smtpsecure": "ssl",
        "mail_smtpauthtype": "LOGIN",
        "mail_smtpauth": 1,
        "mail_smtphost": "***REMOVED SENSITIVE VALUE***",
        "mail_smtpport": "465",
        "mail_smtpname": "***REMOVED SENSITIVE VALUE***",
        "mail_smtppassword": "***REMOVED SENSITIVE VALUE***",
        "allow_local_remote_servers": true,
        "trashbin_retention_obligation": "15, 180",
        "app_install_overwrite": [
            "suspicious_login"
        ],
        "serverinfo": {
            "token": "***REMOVED SENSITIVE VALUE***"
        },
        "trusted_proxies": "***REMOVED SENSITIVE VALUE***",
        "remember_login_cookie_lifetime": "35",
        "session_keepalive": "false",
        "session_lifetime": "30",
        "auto_logout": "true",
        "overwrite.cli.url": "https:\/\/dev-nc.mydomain.tld",
        "theme": "",
        "session_relaxed_expiry": "false",
        "updater.release.channel": "stable",
        "enabledPreviewProviders": [
            "OC\\Preview\\MP3",
            "OC\\Preview\\TXT",
            "OC\\Preview\\MarkDown",
            "OC\\Preview\\OpenDocument",
            "OC\\Preview\\Krita",
            "OC\\Preview\\Imaginary"
        ],
        "preview_imaginary_url": "http:\/\/dev-nextcloud-imaginary:9000",
        "preview_concurrency_all": "12",
        "preview_concurrency_new": "8",
        "log_rotate_size": 52428800
    }
}

occ config:user_oidc

{
    "apps": {
        "user_oidc": {
            "allow_multiple_user_backends": "0",
            "enabled": "yes",
            "id4me_enabled": "0",
            "installed_version": "1.3.5",
            "provider-1-jwksCache": "",
            "provider-1-jwksCacheTimestamp": "1655721705",
            "provider-2-jwksCache": "***REMOVED SENSITIVE VALUE***",
            "provider-2-jwksCacheTimestamp": "1694160061",
            "provider-3-jwksCache": "***REMOVED SENSITIVE VALUE***",
            "provider-3-jwksCacheTimestamp": "",
            "provider-4-jwksCache": "***REMOVED SENSITIVE VALUE***",
            "provider-4-jwksCacheTimestamp": "1691150610",
            "provider-5-jwksCache": "***REMOVED SENSITIVE VALUE***",
            "provider-6-jwksCacheTimestamp": "1701362243",
            "provider-7-bearerProvisioning": "0",
            "provider-7-checkBearer": "0",
            "provider-7-extraClaims": "preferred_username",
            "provider-7-groupProvisioning": "1",
            "provider-7-jwksCache": "***REMOVED SENSITIVE VALUE***",
            "provider-7-jwksCacheTimestamp": "1705524002",
            "provider-7-mappingDisplayName": "name",
            "provider-7-mappingEmail": "email",
            "provider-7-mappingGroups": "x-grants",
            "provider-7-mappingQuota": "",
            "provider-7-mappingUid": "preferred_username",
            "provider-7-providerBasedId": "0",
            "provider-7-sendIdTokenHint": "0",
            "provider-7-uniqueUid": "0",
            "types": "authentication"
        }
    }
}

BTW: current provider is 7 (the only existing provider) but there are still leftovers of previously existed IdP..

@isdnfan
Copy link

isdnfan commented Jan 18, 2024

hi @julien-nc still no success :(

a don't spot any difference to the previous state.. I add the new HAR file as well

dev-nc.mydomain.tld_Archive [24-01-18 21-55-39].har.zip

image

@julien-nc julien-nc force-pushed the fix/743/avoid-logout-login-loop branch from 96aa201 to bcc2b38 Compare January 19, 2024 14:00
@isdnfan
Copy link

isdnfan commented Jan 21, 2024

I'm sorry @julien-nc I still exactly the same issue.. still /logout?requesttoken.. followed by /login?redirect_url=/logout?requesttoken.. I don't see any changes with the last commit. I have now idea if something is wrong with my system - can you advice please?

…_multiple_user_backends == 0)

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc julien-nc force-pushed the fix/743/avoid-logout-login-loop branch from bcc2b38 to 17d7e7f Compare January 23, 2024 15:44
@juliusknorr
Copy link
Member

@julien-nc I think the code change makes sense, but with the config @isdnfan describes I see an issue as there is no singleLogoutService URL defined, so we cannot logout at the IdP and therefore are logged in back right away. However without that I also don't see a good solution except having another intermediate login step with a button that the user needs to click (or do not perform the automatic redirect to the provider when auto_logout is set.

@isdnfan
Copy link

isdnfan commented Feb 2, 2024

hi @juliushaertl thank you for your response.. there is no intention to logout the user from IdP when NC session ends. the idea is exactly as you describe - after session ends on NC side the user immediately login again using IdP..
the problem is new login remembers previous logout URL and immediately proceed to the logout URL.. I'm wondering there is a way to catch such condition and simply replace /login?redirect_url=/logout... with start page?

Or maybe I'm doing something wrong in general? what is the right way to move the "login governance" completely to IdP? in other words I want my IdP to keep track about user session and NC should check very frequently if the user is logged in to IdP and deny access as soon there is no valid IdP login?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants