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 session token creation "remember" parameter #51

Merged
merged 1 commit into from
Nov 29, 2021

Conversation

julien-nc
Copy link
Member

When the slave controller creates the session token on login, the remember parameter is set to IToken::DO_NOT_REMEMBER.

This has a visible negative side effect: It is impossible to create an app password (in the web UI) after having logged in via GSS because OC\User\Session because app_password is set in the session in this case.

https://github.com/nextcloud/server/blob/582234322a59e32fd0d220023a260b66a9b205f2/lib/private/User/Session.php#L850-L854

As it is expected to prevent app password creation when authenticated with an app password, I think it should be possible when logging in via GSS.

I don't know if this is an acceptable fix and if it has bad side effects but it solves the app password generation issue.

…rd by the Session class

Signed-off-by: Julien Veyssier <[email protected]>
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Makes sense

@ChristophWurst
Copy link
Member

https://github.com/nextcloud/server/blob/582234322a59e32fd0d220023a260b66a9b205f2/lib/private/User/Session.php#L850-L854

Maybe I'm confused. But we have two different flags for tokens. Temporary/permanent and remember-me. Temporary/permanent lets us distinguish between browser sessions (temporary token that expires) and app passwords (they don't expire). The remember/do-not-remember flag is only used for session tokens. We default to true (there used to be a checkbox on the login page).

So I'm wondering if nextcloud/server#24552 was possibly wrong. I think we should set the app_password session var when \OC\Authentication\Token\DefaultToken::getType equals \OC\Authentication\Token\IToken::PERMANENT_TOKEN 🤔

(there is no getType on IToken right now)

@juliusknorr
Copy link
Member

So I'm wondering if nextcloud/server#24552 was possibly wrong. I think we should set the app_password session var when \OC\Authentication\Token\DefaultToken::getType equals \OC\Authentication\Token\IToken::PERMANENT_TOKEN 🤔

(there is no getType on IToken right now)

Sounds right to only set the session value when using a permanent token. Tough I'd still think that using a remember me token might be expected behaviour (as with a regular login) also when logging in through the global site selector.

@julien-nc
Copy link
Member Author

@ChristophWurst Do you agree with @juliushaertl 's comment? I mean, does it still make sense to create a remember_me token when logging in via the GSS even if we fix Session::tryTokenLogin()?

Here is a PR for what you suggested: nextcloud/server#29729

@julien-nc julien-nc merged commit 40a64b5 into master Nov 29, 2021
@delete-merged-branch delete-merged-branch bot deleted the fix/noid/slave-login-remember-session-token branch November 29, 2021 10:25
mickenordin added a commit to SUNET/nextcloud-custom that referenced this pull request Dec 3, 2021
* Create app passwords: [Ticket#9623982] nextcloud/globalsiteselector#51
* Create email templates using variables set in config.php:  https://github.com/SUNET/drive-email-template
* Add custom button on direct login page:  https://github.com/SUNET/loginpagebutton/
@juliusknorr
Copy link
Member

/backport to stable1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants