Improve admin webui

[mreid-tt]

Some admin web ui improvements

fix date formatting as well as labels for columns and filters

• add formatters to remove microseconds on dates
• add column labels to be used with filters
• add new filters for version and build views
• remove fulfilled TODOs

fix screenshot management

• remove edit screen (move delete function)
• add file removal to deletion of row in list
• fix default sort order

fix architecture management

• remove builds column from create form

Other admin improvements

fix app configuration (leftover from refactor #102)

• separate babel declaration and initialisation
• remove flake8 exception

fix handling of new service dependencies (fixes #39)

• add service view to admin interface
• add service check and error handling on package upload

fix maintenance of md5 hashes

• add functions to generate md5 hash in build model and spk
• use function when uploading build
• use function when signing/unsigning build

add major_version check (fixes #63)

• add type column to firmware table
• identify the major DSM version based on a closest match to the build, filtered by type
• filter package versions based on match to major DSM version
• include earlier "noarch" package version when major DSM version < 6
• update nas tests for fetching lower build number (arch and noarch)

other/environment fixes

• fix alembic setup environment
• update build model to match database
• fix depopulate db function
• increase length of version column
• update populate_db with firmware type

[mreid-tt]

@hgy59, I was inspired by the work you did on #111 and had submitted some improvements for the admin screens. This mostly treats with labels on columns and filters, addition of new filters and fixing of date formatting.

I had a thought to fix the screenshot management as well but I couldn't make heads or tails of the code. Essentially, I think that the edit function is redundant since you can only really delete the file there. This is problematic since it throws an SQL exception as follows:

Integrity error. (psycopg2.errors.NotNullViolation) null value in column "path" of relation "screenshot" violates not-null constraint DETAIL: Failing row contains (8, 4, null). [SQL: UPDATE screenshot SET path=%(path)s WHERE screenshot.id = %(screenshot_id)s] [parameters: {'path': None, 'screenshot_id': 8}] (Background on this error at: https://sqlalche.me/e/14/gkpj) 

After the file is deleted you are left with a blank screenshot like this (second entry):

From there you can just delete the row. If you delete the row independently, the file isn't deleted. As such, I am proposing that we remove the edit screen altogether and add the delete function to the row deletion code.

EDIT: I found a simple solution in the Flask-Admin docs and the example code for image uploader was useful.

[mreid-tt]

@publicarray @hgy59, this PR should be good to merge. Let me know if there are any improvements you would recommend.

[mreid-tt]

@Diaoul, I was looking at the backend code as well as the database and saw that there is a field in the database build/md5 which doesn't seem to get populated when I upload a package. I see that it is referenced in the nas.py as part of a response of entries but I don't see where in the api.py that it is created.

I do however see that is is created as part of the test suite:

spkrepo/spkrepo/tests/common.py

Lines 281 to 286 in ec69e1e

	with create_spk(self) as spk_stream:
	self.save(spk_stream)
	if self.md5 is None:
	spk_stream.seek(0)
	self.md5 = hashlib.md5(spk_stream.read()).hexdigest()
	spk_stream.close()

Should code similar to the above be incorporated into the api.py on package upload?

EDIT: I've included a proposed solution in 688bcfd, let me know what you think.

[Diaoul]

Good catch! It's been like that forever 😅

[mreid-tt]

Good catch! It's been like that forever 😅

I've tested the upload and it writes a hash value to the database which I've verified to be correct. I was trying to check the sign and unsign functions but I don't think I have my environment setup correctly.

From the base config.py, I've set the GNUPG_PATH = "/usr/bin/gpg" and I've set the SECRET_KEY to a random string generated by base64 < /dev/urandom | head -c 64 but when I try to sign any package I am getting a "failed to sign build" error with no other log details. Can you assist?

EDIT: So I ran all the verification tests again and I got this error:

        # issue 112: fail if the specified value isn't a directory
        if gnupghome and not os.path.isdir(gnupghome):
>           raise ValueError('gnupghome should be a directory (it isn\'t): %s' % gnupghome)
E           ValueError: gnupghome should be a directory (it isn't): /usr/bin/gpg

So then I changed it to GNUPG_PATH = "/usr/bin" and ran it again. This time I got this error:

  File "/home/mreid/Documents/spkrepo/spkrepo/utils.py", line 369, in _generate_signature
    raise SPKSignError("Cannot verify timestamp")
spkrepo.exceptions.SPKSignError: Cannot verify timestamp

So, I'm not sure where to go next. Is the default value no longer valid: GNUPG_TIMESTAMP_URL = "http://timestamp.synology.com/timestamp.php"?

[Diaoul]

This should be a GNUPGHOME directory, not /usr/bin 😬

Any directory would work but please create a new one.

Maybe you need to create a private/public key pair I don't remember. I'd need to check the configuration on the server 🙇

[mreid-tt]

This should be a GNUPGHOME directory, not /usr/bin 😬

Thanks for that. I eventually used the default folder at /home/mreid/.gnupg and that worked fine to interact with the default keystore.

Maybe you need to create a private/public key pair I don't remember. I'd need to check the configuration on the server 🙇

While creating a private/public key pair was a useful step, this was not the root cause of the issue. The Cannot verify timestamp error was because it could not validate the signature returned from the timestamp server. To address this I had to import its public keys like this:

$ curl https://keymaker.synology.com/synokey --output synokey.tar
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 10240  100 10240    0     0   8169      0  0:00:01  0:00:01 --:--:--  8172

$ tar -xvf synokey.tar
VERSION
keyinfo-sys
keyring

$ gpg --import keyring 
gpg: key 38E86A2B309E88B0: public key "Synology Inc. (Timestamp)" imported
gpg: key E92D287366A13D20: public key "Synology Inc. (Official)" imported
gpg: Total number processed: 2
gpg:               imported: 2

Once those steps were completed I was able to sign and unsign packages successfully. Through this, I fixed an error with updating the md5 hash for the sign/unsign and the commit was updated. Testing looks good so far.

[th0ma7]

Probably a bit out of topic but out of curiosity, using this, are you able to unsign an official Synology package as well, thus allowing to untar afterwards?

[mreid-tt]

Probably a bit out of topic but out of curiosity, using this, are you able to unsign an official Synology package as well, thus allowing to untar afterwards?

Alas, no. The official packages are likely encoded or encrypted in some way to prevent simple untar operations. If I am curious about a package (to learn from its wizard files for example), the most I would do is to install it and then look at the package files installed in /var/packages/[package_name].

[hgy59]

@mreid-tt what is the reason to enlarge the version field?

• hopefully not to add more parts to the version (major.minor must be sufficient). In fact only the major version is relevant, the main criteria for version compatibility is the build-version.

I guess you are about to implement the feature to avoid delivering DSM 6 packages for DSM 7 requests?

[mreid-tt]

@mreid-tt what is the reason to enlarge the version field?

• hopefully not to add more parts to the version (major.minor must be sufficient). In fact only the major version is relevant, the main criteria for version compatibility is the build-version.

Currently the field was of length 3 characters. I expect that this will cause a problem after Synology moves from version 9.9 to version 10.0.

I guess you are about to implement the feature to avoid delivering DSM 6 packages for DSM 7 requests?

Yes, I've made a basic implementation in e2b7f29 which works as follows:

1. We add a minimum compatible DSM version field to the firmware table
   • So for example if we have firmware with version 7.0 or 7.1 we can set a minimum version as 7.0
2. This will force the NAS function to omit builds in the catalog which do not meet this minimum

@hgy59, @publicarray, let me know your thoughts on this solution to #63.

EDIT: It's a bit clunky because if you choose to set a minimum version, you'll need to set it for every firmware with the same version number but it works!

[hgy59]

Your comment above from the original logic proposal gave me an idea. I am thinking that in the Firmware table we add a field named 'type' that would be a string that would contain the family of the software ('dsm' or 'srm'). That way I could adjust the query to be something like:

good idea,
(we might use a boolean field to enable/disable the the firmware entries for delivery by API)

[mreid-tt]

I've implemented the idea, let me know your thoughts.

(we might use a boolean field to enable/disable the the firmware entries for delivery by API)

I do still think we need a type which is clearer for the administrator to understand. What I can do as a creature comfort would be to tweak the admin interface to make that a dropdown selector rather than typing it in.

EDIT: Changed my mind on that last bit. Seems overly complex to implement and maintain.

EDIT: Added some input validation with regex to help keep our database clean.

[mreid-tt]

@Diaoul, @publicarray, could you please take a moment to review and approve this? I'm excited to move forward and get these into production. Your feedback is greatly appreciated.

[mreid-tt]

@Diaoul, @publicarray, could either of you find a moment to review and approve this? There have been increasing reports on Discord regarding users encountering difficulties downloading incorrect packages from the catalog.

[mreid-tt]

@Diaoul, @publicarray, I've noticed an increase in support requests on Discord due to the backend displaying the wrong package for download. Could you please prioritise the review and merging of this PR? It's urgent.

[publicarray]

Thanks I hope to have some time this weekend to look into deploying this pr

[mreid-tt]

Thanks I hope to have some time this weekend to look into deploying this pr

Really appreciate it! If you have time it would be great if you can include a review and merge of #111 as this also includes a number of important fixes.

[publicarray]

Nice work!

Running flask db upgrade gives this error:

  self.pop(exc_value)
  File "/home/seb/.cache/pypoetry/virtualenvs/spkrepo-4s3rbOEh-py3.11/lib/python3.11/site-packages/flask/ctx.py", line 261, in pop
    raise AssertionError(
AssertionError: Popped wrong app context. (<flask.ctx.AppContext object at 0x7f0425c22e10> instead of <flask.ctx.AppContext object at 0x7f0428915710>)

I'll have a look what happened here.

[publicarray]

At first, I got this 'NoneType' object has no attribute 'strftime'

So I imported datetime until I realised, you would get this error when modifying the confirmed_at value as it's not a string but a custom object from Flask-SQLAlchemy:

BaseModelView.index_view() got an unexpected keyword argument 'cls'

    column_formatters = {
        "confirmed_at": lambda v, c, m, p: m.confirmed_at + "foo"
    }

[mreid-tt]

@publicarray, I'm thinking that the 'NoneType' object has no attribute 'strftime' is because we have values in the table of None for the confirmed at. Modifying the code to this may resolve it:

column_formatters = {
    "confirmed_at": lambda v, c, m, p: m.confirmed_at.strftime("%Y-%m-%d %H:%M:%S") if m.confirmed_at else None
}

[publicarray]

Thanks @mreid-tt that fixed it!

[mreid-tt]

@publicarray, I've re-committed the change. Let me know if there is anything else I can assist with to get this PR merged.

[publicarray]

@mreid-tt

I've tried uploading a package freshly compiled from spkrepo am I missing something?

http --verify=no --ignore-stdin --auth XXXXX: POST http://127.0.0.1:5000/api/packages @packages/transmission_x64-7.1_4.0.5-26.spk 
HTTP/1.1 409 CONFLICT
Connection: close
Content-Length: 220
Content-Type: application/json
Date: Sun, 25 Feb 2024 10:18:09 GMT
Server: Werkzeug/3.0.1 Python/3.11.7
Vary: Cookie

{
    "message": "Conflicting architectures: apollolake, avoton, braswell, broadwell, broadwellnk, broadwellnkv2, broadwellntbap, bromolow, cedarview, denverton, epyc7002, geminilake, grantley, kvmx64, purley, r1000, v1000"
}

[mreid-tt]

I've tried uploading a package freshly compiled from spkrepo am I missing something?

Hmm, I don't think I modified any of the conflict testing code. Could this be an actual conflict and there is really a x64-7.1 type build already present in your server for version 4.0.5-26? Could you check what variations of Transmission already exist?

[publicarray]

I tried again with 7.2 and that worked. Yea it probably already existed, sorry

[publicarray]

I think it's all good :)

[publicarray]

@mreid-tt It's live

I did notice this in the logfile when I login. Doesn't seem important but maybe there is an update for passlib?

synocommunity-app-1  | (trapped) error reading bcrypt version
synocommunity-app-1  | Traceback (most recent call last):
synocommunity-app-1  |   File "/usr/local/lib/python3.11/site-packages/passlib/handlers/bcrypt.py", line 620, in _load_backend_mixin
synocommunity-app-1  |     version = _bcrypt.__about__.__version__
synocommunity-app-1  |               ^^^^^^^^^^^^^^^^^
synocommunity-app-1  | AttributeError: module 'bcrypt' has no attribute '__about__'

[mreid-tt]

@mreid-tt It's live

Thanks @publicarray, this is great!

I did notice this in the logfile when I login. Doesn't seem important but maybe there is an update for passlib?

Looking into this it seems that passlib 1.7.4 is the latest version and it seems like it's no longer maintained. This was reported as a known issue and there is a fork that seems to resolve the issue. Given that it appears to be just a cosmetic logging issue we should be able to leave it alone for now.

[mreid-tt]

BTW: the productive db might need some mantenance. The following (invalid) firmware version has to be removed

• 7.0-4000 (the 7.0-40000 must be kept)

AFAICR that was me who accidentally added that.

As a follow-up, I have worked with @publicarray and we have removed the following firmware entries from the database:

Version	Build	Type
1.2	1757	srm
7.0	4000	dsm
6.2	22259	dsm
7.2	63134	dsm

These were removed as they did not align with the published versions of DSM (https://www.synology.com/en-global/releaseNote/DSM) or SRM (https://www.synology.com/en-global/releaseNote/SRM).