search

threefoldtech / 0-bootstrap

Zero-OS Bootstrap Webservice
Apache License 2.0
1 stars 2 forks source link

Farm ID legitimacy check #30

Closed Kaya-Sem closed 4 months ago

Kaya-Sem commented 4 months ago

:warning: Needs extra testing: someone other than me should test this as well.

Issue

In the current implementation, the farm ID textfield does not check the given ID. This makes it easy to enter the wrong one without knowing any better. This is bad practice in general, and can cause cryptic issues down the line with ZOS. I have personally used a wrong farm ID by accident and thus had issues with my node, and the cause was not instantly clear.

This is an extra step in ensuring no node is created with a wacky farm ID

image

Fix

To combat this issue, I've implemented a simple fetch from the Threefold GraphQL database to verify a farm exists with the given ID.

As a QOL change I made the right label show the name for the farm, as an extra measure to ensure correct ID. To fix the issue of retrieving the ID from that field, I introduced a variable that holds the ID, and the URL gets generated from that.

https://github.com/threefoldtech/0-bootstrap/assets/73200952/3cb91076-6ac9-4365-abc8-104f2a4178f1

(reload page if giving error)

Sanitizing input

issue #4 was not implemented fully, making it possible to still input dots (even though these are removed before use). This is now also taken care of.

Kaya-Sem commented 4 months ago

For performance reasons, the declaration of the endpoint and the query can be extracted, so they are not re-initialized for every trigger.

maxux commented 4 months ago

Thanks, I quickly checked your pull request and it seems clean. I'll double read it monday and will probably merge it :)

maxux commented 4 months ago

I'm going to test it asap then merge if everything seems good but code review looks good ! :)

maxux commented 4 months ago

Tested, overall workflow works as expected.

Some remarks, to be able to run the code, locally I have a missing dependency for Markup (from flask import Markup). After a double check, Markup is not used on the code so the import can be dropped. There are also 1 or 2 extra space on the code but nothing important, I'm gonna fix that later :)

maxux commented 4 months ago

It's in production. Thanks !

I just thinked about something, now you use graphql to validate the farmer but there are different graphql endpoint depending on the network (test and dev have their own graphql and own farm ids). That would be interesting to support theses case :)