alexbelgium
commented
1 day ago Hi, thanks I see your point. I'll check tomorrow (an update will also likely come this night as a new version exists)
Open IPTN opened 1 day ago
alexbelgium
commented
1 day ago Hi, thanks I see your point. I'll check tomorrow (an update will also likely come this night as a new version exists)
alexbelgium
commented
1 day ago The issue is that if there is no password it is less convenient for novice users for whom it would be easier to have the password set by default in the HA options. Perhaps I could add a specific boolean that would be automatically reset to false on boot, allowing to circumvent the whole password feature for one single boot...
IPTN
commented
1 day ago Based on the documentation for addon configuration, removing the password field from schema but keeping it in options with the current default value should result in what I understand to be the desired behavior documented in the README. No other changes should be necessary unless you are looking to change functionality.
That change will make the password field optional while still retaining the default value.
I tried this change locally and it did not work as I expected. It did allow reaching the Initial Setup screen for Portainer.... because password was no longer being used from options at all.
With password in options and not schema:
At this point I figured that the documentation was incorrect. There was a relevant line showing in Supervisor logs WARNING (MainThread) [supervisor.addons.options] Option 'password' does not exist in the schema for Portainer (db21ed7f_portainer).
I found the relevant HA code: https://github.com/home-assistant/supervisor/blob/d09460a97170f239f0247f3a87692b39d45b09f6/supervisor/addons/options.py#L83-L92 There is that log line (I didn't actually look at the Supervisor log until I saw it here). And the comment indicates that this is how it is intended to work.
The other part of the code that is important to the functionality that is trying to be achieved with the password field is here: https://github.com/home-assistant/supervisor/blob/d09460a97170f239f0247f3a87692b39d45b09f6/supervisor/addons/options.py#L256-L261 It looks like it should work as currently implemented given the check for it being defined in the schema as a string ending in '?'. This matches the documentation.
Well the problem is in the edge case as always. It never actually makes it to that point because of an earlier validation called just before it checks for missing options: https://github.com/home-assistant/supervisor/blob/d09460a97170f239f0247f3a87692b39d45b09f6/supervisor/addons/options.py#L116-L120 You'll notice it doesn't actually check if the schema field is optional when throwing the error for a null value; despite the comment seemingly indicating it should only be applied to required fields.
The last piece is pretty obvious. It is not possible to just remove the field from the addon options (which I guess is how all optional fields currently work) because it is populated with the default value before the schema validation is even performed: https://github.com/home-assistant/supervisor/blob/d09460a97170f239f0247f3a87692b39d45b09f6/supervisor/addons/addon.py#L963-L973
To circle back around, the documentation was not clear enough to me on the several times I read it before looking at the actual implementation.
The options dictionary contains all available options and their default value. Set the default value to null or define the data type in the schema dictionary to make an option mandatory. This way the option needs to be given by the user before the add-on can start. ~ To make an option truly optional (without default value), the schema dictionary needs to be used. Put a ? at the end of the data type and do not define any default value in the options dictionary. If any default value is given, the option becomes a required value. ~ The schema looks like options but describes how we should validate the user input.
It's not obvious that all fields used need to be defined in schema, and the documentation on optional fields is not well organized/is written in seperate paragraphs with somewhat conflicting information and with differing terminology.
The defining part for this use case is "If any default value is given, the option becomes a required value.", which didn't really sink in until going down this rabbit hole.
T.L.D.R. So in summary, it is not currently possible to have an optional field with a default value because you fields can never be set to None/null regardless of optional status and in spite of comments in the code indicating otherwise. (And I am of the opinion that documentation on addon options and schema is not clear enough, especially as regards to optional fields).
^ None of which is a bug inside of your addon, with the small caveat that the optional flag for password won't work with a default value. Right back where I started: https://github.com/alexbelgium/hassio-addons/blob/25e8a1c324ac6c9ff63f7622f0706a6373e39d28/portainer/config.json#L44
alexbelgium
commented
21 hours ago Now you are a very comprehensive person in your search and documentation!
My slight concern in removing the default value is that it makes things less "beginner-proof" as the password will need to be set by them though an action.
Would a middle ground : typing "blank" in the password field be an acceptable workaround? And I'll update the documentation as such. This would allow both the use case of starting without a password (resetting the database), and setting a default value for beginners to have a ready-to-use add-on.
Second option for me is to remove the default option tag but make a big warning in the log that the password was not set in the options
IPTN
commented
14 hours ago I understand the intention behind the feature, but I would like to point out that when starting Portainer without setting the password results in the Initial Setup being shown to the user immediately on first startup.
So my prefence would be to make use of the Portainers built-in onboarding.
One downside to relying on Portainers Initial Setup exclusively is that a beginner user has no way of easily retrieving or reseting the password outside of Portainer. The easiest resolution for such a user in that situation would likely be to simply reinstall the addon and perform initial setup again. This obviously results in their configuration being lost, but that is not materially different than the current implementation that wipes when the password is changed through the password field. Still, this could be addressed by changing to a reset_password field which when set results in the current behavior of a password change and is then cleared with bashio.
The other caveat with this approach is that there is a 5 minute window to complete the Initial Setup. If this timeout is reached it results in a rather unhelpful page that only displays '404: Not Found'. It is easily resolvable though by looking at the log: INF github.com/portainer/portainer/api/adminmonitor/admin_monitor.go:62 > the Portainer instance timed out for security purposes, to re-enable your Portainer instance, you will need to restart Portainer |.
The reason for the very unhelpful page shown is due to Portainer redirecting to `http://{homeassistant.local:8123}/timeout.html', which obviously doesn't work due to the reverse proxy. Unfortunately, I don't think this is easily mitigated to properly serve the intended timeout page or a replacement, but the log message is clear.
The alternative middle ground you suggested would certainly work. However, for a feature targeted at beginners I believe it has some downsides.
First, that it is not obvious that changing the password in this manner will reset Portainer, which is currently mitigated with the backup and log messages indicating such after the fact (this behaviour should probably be included with the documentation change if you decide to go with this approach). I would argue, however, that a beginner who needs this feature would not easily be able to restore to the backup inside the container.
The other pitfall that exists is that the password is kept in the config directly and currently is being logged: The check to not log if the field contains 'PASS' is not met with the current fieldname: https://github.com/alexbelgium/hassio-addons/blob/38a9814136233919d0d993c1d0f4ba2d9d7e2c3c/.templates/00-global_var.sh#L57-L60 But, it is intentional behaviour for this addon in particular. https://github.com/alexbelgium/hassio-addons/blob/38a9814136233919d0d993c1d0f4ba2d9d7e2c3c/portainer/rootfs/etc/services.d/portainer/run#L57 Since the container is reversed proxied behind the HA interface, and therefore a valid HA login, it is not immediately abusable if leaked. But for a beginner it would be fairly easy to accidently leak either through sharing their config (unlikely to be using secrets.yaml) or sharing their log which could be a problem if it is a credential they are reusing.
Regardless, my understanding is that this addon specifically meant for more advanced users due to the potential for footguns: The description provided with the addon (and I believe the stated reason for its removal from the community addons). It is obviously your decision to make the addon as accessible as possible (especially with adding guard rails), so feel free to ignore my 2cents; But, a user who is unable to complete first time setup (which IMO is a fairly low barrier to entry) without this feature might be better served not using this addon until they are more experienced.
Thanks for your work maintaining this addon and being so responsive!
Description
Currently it is not possible to perform Initial Setup of Portainer which prevents restoring from backup. https://github.com/alexbelgium/hassio-addons/blob/25e8a1c324ac6c9ff63f7622f0706a6373e39d28/portainer/config.json#L44 This line is preventing removing the password from the addon configuration options, which is the way to cause a full database reset and allow initial setup. https://github.com/alexbelgium/hassio-addons/blob/25e8a1c324ac6c9ff63f7622f0706a6373e39d28/portainer/README.md?plain=1#L76 https://github.com/alexbelgium/hassio-addons/blob/25e8a1c324ac6c9ff63f7622f0706a6373e39d28/portainer/rootfs/etc/services.d/portainer/run#L53-L61
This behavior of not allowing matches HA documentation for the options and schema fields, which results in this behavior
When simply removing the password field from the addon options it will be repopulated to the default value of 'homeassistant'.
https://github.com/alexbelgium/hassio-addons/blob/25e8a1c324ac6c9ff63f7622f0706a6373e39d28/portainer/config.json#L28
Removing the line first referenced from schema in config.json should result in the intended behavior of allowing not setting a password in the addon options.
Willing to submit a PR for this change if requested, but it would obviously be a trivial one.
Reproduction steps
Addon Logs
Architecture
No response
OS
No response