Skip to content

Conversation

@hamza221
Copy link
Contributor

@hamza221 hamza221 commented May 15, 2025

Fix #809

The existing logic checks the authtoken table which comes with the tradeoff that user's row is deleted on logout.

@hamza221
Copy link
Contributor Author

/backport to stable31

@hamza221
Copy link
Contributor Author

/backport to stable30

@kesselb
Copy link
Collaborator

kesselb commented May 15, 2025

Thank you 🙏

I will review and test asap.

@tcitworld what do you think?

@hamza221
Copy link
Contributor Author

I'll check the failing oracle test tomorrow

Copy link
Member

@tcitworld tcitworld left a comment

Choose a reason for hiding this comment

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

Funny enough, I've come to the same realization on the very same day this PR comes out.

Worst thing is probably people using WebDAV sessions which are deleted from oc_authtoken after one day.

->where($queryBuilder->expr()->eq('appid', $queryBuilder->createNamedParameter('login')))
->andWhere($queryBuilder->expr()->eq('configkey', $queryBuilder->createNamedParameter('lastLogin')))
->andwhere($queryBuilder->expr()->gte(
$queryBuilder->expr()->castColumn('configvalue', IQueryBuilder::PARAM_INT),
Copy link
Collaborator

Choose a reason for hiding this comment

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

OCI tests are failing because configvalue is a string and the other argument an integer.

My commit to cast configvalue to an int made Postgres and OCI unhappy 😆

Copy link
Collaborator

Choose a reason for hiding this comment

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

$queryBuilder->expr()->gte(
				'configvalue',
				$queryBuilder->createNamedParameter((string)$this->timeFactory->getTime() - $offset),
				IQueryBuilder::PARAM_STR,
			)

Treating both as string seems to work, but isn't that weird?

@kesselb kesselb force-pushed the fix/active-users branch 2 times, most recently from 3a52433 to 505c843 Compare May 21, 2025 16:32
Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>
@kesselb kesselb force-pushed the fix/active-users branch from 4bfc049 to 83f7802 Compare May 22, 2025 08:05
@kesselb kesselb enabled auto-merge May 22, 2025 08:07
@kesselb kesselb merged commit 98732ef into master May 22, 2025
39 checks passed
@kesselb kesselb deleted the fix/active-users branch May 22, 2025 08:07
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.

Inaccurate statistics for active usage

4 participants