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 passwordless app token generation #29187

Merged
merged 2 commits into from
Oct 13, 2021

Conversation

julien-nc
Copy link
Member

Even with #29122, we still face the app token invalidation issue after 5 minutes.

An app token generated in AppPasswordController with an empty string as the password parameter has a non empty password. It can be observed there

if ($savedToken->getPassword() === null) {

when using the token like that for example:
curl -H "Authorization: Bearer APP_TOKEN_GENERATED_USING_OIDC_TOKEN_TO_AUTHENTICATE" "https://what.e.ver/ocs/v1.php/cloud/users/number6" -H "OCS-APIRequest: true"

I don't know if this is a proper way to fix this but setting a null password when generating an app token seems a safe way to make it passwordless and fixes the invalidation issue.

@ChristophWurst
Copy link
Member

This indeed seems to rather fix the symptoms than the causes. As I understand the problem is that the password is set to an empty string when no password is available for OIDC. The real solution is to set the password to null for those sessions. Do you know where we create those tokens?

@juliusknorr
Copy link
Member

For that it would be

$userSession->createSessionToken($request, $uid, $uid);
which is a bit odd there as it creates the session token with null, but maybe through the login hook/event below the token gets updated or a new sessino token gets created.

Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
@julien-nc julien-nc force-pushed the fix/noid/passwordless-app-password-generation branch from c5fcea3 to de5fea4 Compare October 13, 2021 10:25
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
@julien-nc
Copy link
Member Author

The real solution is to set the password to null for those sessions.

Very true 😁.

which is a bit odd there as it creates the session token with null, but maybe through the login hook/event below the token gets updated or a new sessino token gets created.

Same conclusion for me. Correctly setting the password to null in OC_Hook::emit() fixes the issue.

But the password can't be set to null in the UserLoggedInEvent constructor because of the type constraints. I pushed a second commit here allowing a null password in the event. It might break something further. It seems to work fine considering how the credentials manager stores and retrieve the credentials.

@ChristophWurst
Copy link
Member

It might break something further. It seems to work fine considering how the credentials manager stores and retrieve the credentials.

Let's document this at #27846. PhpStorm doesn't find any other usages but the credentials update listener. The non-nullable password was a mistake when the event was created. Passwords have always been optional.

@juliusknorr juliusknorr merged commit 581862b into master Oct 13, 2021
@juliusknorr juliusknorr deleted the fix/noid/passwordless-app-password-generation branch October 13, 2021 14:11
@summersab
Copy link
Contributor

I'm concerned with this fix. Do a quick search of the server source for string $password, and you will see over a dozen core libraries and functions that typecast the password argument as a string. Setting the password to null may lead to issues similar to the conversation I'm having here: #27929 (comment). I agree with Christoph that this may be a fix of the symptoms rather than the root cause.

That said, smarter minds than mine should probably chime in.

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.

5 participants