-
Notifications
You must be signed in to change notification settings - Fork 3
feat: add daily transmission of config report to ownCloud/kiteworks f… #197
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
Conversation
c420281 to
7762115
Compare
|
| private function getBasicDetailArray(string $licenseKey): array { | ||
| return [ | ||
| 'license key' => $this->licenseKey, | ||
| 'license key' => $licenseKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe I'm missing something, but I don't see the value change... why bother to include a "license key" field when it will always contain IConfig::SENSITIVE_VALUE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the license key is transmitted via the http internface. It is necessary as some kind of identifier for customers.
lib/Telemetry/Monitor.php
Outdated
|
|
||
| $client = $this->clientService->newClient(); | ||
| try { | ||
| $client->post('https://3qyh826rmd.execute-api.ap-southeast-1.amazonaws.com/default/pocTelemetry', [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make it configurable?
The use case would be, if we ever need to change the location, old versions would still send the data to the old endpoint without a chance to send it to the right one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working for a US based storage company in the past having a similar telemetry report (call home) function, you must have an unchangeable target address. But I propose to have in addition a configurable value where you can add multiple other addresses like via email. So checking if email is configured, you send additional reports to addresses defined in that key. Topic: "Telemetry report from ". Just my 2c
| } | ||
|
|
||
| private function collectTelemetryData(): array { | ||
| $licenseKey = $this->getLicenseKey(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check for the license state? (https://github.com/owncloud/core/blob/master/lib/public/License/ILicenseManager.php#L95)
We might not be interested in invalid or expired licenses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above - it is used as an identifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The license can be changed anytime, specially if it's about to expire. Any instance could have multiple licenses during its lifetime (one at a time). Using the license as an identifier doesn't seem good.
In addition, I think we should consider the license as a password in regards of confidentiality. I mean, even if leaking the license might be harmless for the instance, there could be other instances taking advantage of the leaked license. If we need to send it, we probably should either hash it or encrypt it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All known and understood. The current implementation was discussed and agreed on offline with different parties.
Nothing we need to discuss in here again. THX a lot for your comments!
189f8a0 to
c86af77
Compare
7d350d1 to
6947daf
Compare
426d426 to
ccfcea1
Compare
…or business intelligence
5382324 to
ead6828
Compare
|



…or business intelligence
Note
Explicit Release note required for community users
Explicit documentation notice required for community users
@mmattel
Example Telemetry Content
{ "id": "ocrq6cgpjlqj", "report": { "basic": { "license key": "", "date": "Mon, 18 Mar 2024 11:25:09 +0000", "ownCloud version": "10.14.1.0", "ownCloud version string": "10.14.1 prealpha", "ownCloud edition": "Community", "server OS": "Linux", "server OS version": "Linux deepdiver 6.6.15-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.6.15-2 (2024-02-04) x86_64", "server SAPI": "cli", "webserver version": "???", "hostname": "???", "logged-in user": "" }, "stats": { "users": { "Database": { "total_count": 1, "guest_count": 0, "seen": 0, "logged in (30 days)": 0 } }, "groups": { "OC\\Group\\Database": 1 } }, "config": { "telemetry.enabled": true, "passwordsalt": "***REMOVED SENSITIVE VALUE***", "secret": "***REMOVED SENSITIVE VALUE***", "trusted_domains": [ "localhost" ], "datadirectory": "/home/deepdiver/Development/owncloud/core/data", "overwrite.cli.url": "http://localhost", "dbtype": "sqlite3", "version": "10.14.1.0", "allow_user_to_change_mail_address": "", "logtimezone": "UTC", "apps_paths": [ { "path": "/home/deepdiver/Development/owncloud/core/apps", "url": "/apps", "writable": false }, { "path": "/home/deepdiver/Development/owncloud/core/apps-external", "url": "/apps-external", "writable": true } ], "installed": true, "instanceid": "ocrq6cgpjlqj", "loglevel": 2, "maintenance": false }, "phpinfo": [] } }