Skip to content
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

Update samba Modul #1187

Merged
merged 19 commits into from Nov 7, 2023
Merged

Update samba Modul #1187

merged 19 commits into from Nov 7, 2023

Conversation

Saharel001
Copy link
Contributor

Modul arbeitet nun mit folgendem Pfadaufbau:
IP oder Hostname/Share/Folder
image

Modul arbeitet nun mit folgendem Pfadaufbau:
IP oder Hostname/Share/Folder
@Saharel001
Copy link
Contributor Author

Saharel001 commented Oct 21, 2023

Die notwendigen Änderungen an der GUI habe ich auch als PR in openWB/openwb-ui-settings#367 angemeldet.

image

Copy link
Contributor

@benderl benderl left a comment

Choose a reason for hiding this comment

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

Danke für den Bugfix und Korrektur. Bitte noch die inline Kommentare beachten.

docs/Samba als Sicherung einrichten.md Outdated Show resolved Hide resolved
packages/modules/backup_clouds/samba/backup_cloud.py Outdated Show resolved Hide resolved
packages/modules/backup_clouds/samba/backup_cloud.py Outdated Show resolved Hide resolved
@benderl benderl added bug Something isn't working ui depends on changes in ui repository labels Oct 23, 2023
@benderl
Copy link
Contributor

benderl commented Oct 23, 2023

Noch eine Ergänzung zu den erlaubten Zeichen (https://learn.microsoft.com/en-us/rest/api/storageservices/naming-and-referencing-shares--directories--files--and-metadata):

The following characters aren't allowed: " \ / : | < > * ?

Demnach sollte nicht nur : ersetzt werden, sondern auch der Pfad auf erlaubte Zeichen geprüft werden und bei ungültigen eine Fehlermeldung ausgegeben werden. Die Eingabe in der GUI sollte ebenfalls durch ein Pattern abgesichert werden.

@MartinRinas
Copy link
Contributor

Noch eine Ergänzung zu den erlaubten Zeichen (https://learn.microsoft.com/en-us/rest/api/storageservices/naming-and-referencing-shares--directories--files--and-metadata):

The following characters aren't allowed: " \ / : | < > * ?

Demnach sollte nicht nur : ersetzt werden, sondern auch der Pfad auf erlaubte Zeichen geprüft werden und bei ungültigen eine Fehlermeldung ausgegeben werden. Die Eingabe in der GUI sollte ebenfalls durch ein Pattern abgesichert werden.

ich hab' auch eine Prüfung im OneDrive Modul, diese ist vermutlich auch unvollständig - wenngleich ich ja nur den Namen der Backupdatei überprüfen muss.

Macht es ggf. Sinn in einem zentralen Modul eine Funktion zur Bereinigung von Dateinamen bereizustellen? soetwas wie sanitize_filenames_for_windows.

Co-authored-by: benderl <benderl@users.noreply.github.com>
@Saharel001
Copy link
Contributor Author

Saharel001 commented Oct 23, 2023

Noch eine Ergänzung zu den erlaubten Zeichen (https://learn.microsoft.com/en-us/rest/api/storageservices/naming-and-referencing-shares--directories--files--and-metadata):

The following characters aren't allowed: " \ / : | < > * ?

Demnach sollte nicht nur : ersetzt werden, sondern auch der Pfad auf erlaubte Zeichen geprüft werden und bei ungültigen eine Fehlermeldung ausgegeben werden. Die Eingabe in der GUI sollte ebenfalls durch ein Pattern abgesichert werden.

Hab nur den : entfernt da openwb2 die Sicherung so erstellt. Eventuell gleich bei der Erstellung der Sicherung darauf achten?!

Der Pfad könnte ja auch abseits von der GUI per MQTT gesetzt worden sein und ungültige Zeichen enthalten. Daher muss im Backend immer eine vollständige Prüfung erfolgen.

@benderl
Copy link
Contributor

benderl commented Oct 23, 2023

ich hab' auch eine Prüfung im OneDrive Modul, diese ist vermutlich auch unvollständig - wenngleich ich ja nur den Namen der Backupdatei überprüfen muss.

Macht es ggf. Sinn in einem zentralen Modul eine Funktion zur Bereinigung von Dateinamen bereizustellen? soetwas wie sanitize_filenames_for_windows.

Wenn es da Parallelen gibt, wäre das durchaus sinnvoll. Ich denke aber, dass OneDrive da nicht so restriktiv ist wie das "gute alte" SMB.

@MartinRinas
Copy link
Contributor

ich hab' auch eine Prüfung im OneDrive Modul, diese ist vermutlich auch unvollständig - wenngleich ich ja nur den Namen der Backupdatei überprüfen muss.
Macht es ggf. Sinn in einem zentralen Modul eine Funktion zur Bereinigung von Dateinamen bereizustellen? soetwas wie sanitize_filenames_for_windows.

Wenn es da Parallelen gibt, wäre das durchaus sinnvoll. Ich denke aber, dass OneDrive da nicht so restriktiv ist wie das "gute alte" SMB.

naja am Ende vermutlich schon, die Dateien werden i.d.R. ja auf Windows-Systemen geöffnet. Hier werden sind diese Zeichen im Dateinamen nicht erlaubt, ist auch in dem Microsoft learn Artikel oben zu finden.

Saharel001 and others added 2 commits October 23, 2023 10:12
conn = SMBConnection(config.smb_user, config.smb_password, os.uname()[1], config.smb_server, use_ntlm_v2=True)
log.info("SMB Verbindungsaufbau")
if conn.connect(config.smb_server,139) == True:
log.warn("SMB Verbindungsaufbau erfolgreich")
Copy link
Contributor

Choose a reason for hiding this comment

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

log.warn sollte auch nur für Warnungen genutzt werden. Analog zu der Zeile 16 hier auch log.info verwenden.

Suggested change
log.warn("SMB Verbindungsaufbau erfolgreich")
log.info("SMB Verbindungsaufbau erfolgreich")

@benderl
Copy link
Contributor

benderl commented Oct 23, 2023

Wenn es da Parallelen gibt, wäre das durchaus sinnvoll. Ich denke aber, dass OneDrive da nicht so restriktiv ist wie das "gute alte" SMB.

naja am Ende vermutlich schon, die Dateien werden i.d.R. ja auf Windows-Systemen geöffnet. Hier werden sind diese Zeichen im Dateinamen nicht erlaubt, ist auch in dem Microsoft learn Artikel oben zu finden.

Ok, guter Punkt. Ich passe das backup.sh Skript so an, dass der Doppelpunkt verschwindet. Für die Prüfung des Pfads oder sonstiger Konfigurationseinstellungen ist aber das jeweilige Plugin verantwortlich.

Wenn dieser PR durch ist, können wir über die angesprochene zentrale Prüfmethode reden.

Co-authored-by: benderl <benderl@users.noreply.github.com>
@Saharel001
Copy link
Contributor Author

Wenn es da Parallelen gibt, wäre das durchaus sinnvoll. Ich denke aber, dass OneDrive da nicht so restriktiv ist wie das "gute alte" SMB.

naja am Ende vermutlich schon, die Dateien werden i.d.R. ja auf Windows-Systemen geöffnet. Hier werden sind diese Zeichen im Dateinamen nicht erlaubt, ist auch in dem Microsoft learn Artikel oben zu finden.

Ok, guter Punkt. Ich passe das backup.sh Skript so an, dass der Doppelpunkt verschwindet. Für die Prüfung des Pfads oder sonstiger Konfigurationseinstellungen ist aber das jeweilige Plugin verantwortlich.

Wenn dieser PR durch ist, können wir über die angesprochene zentrale Prüfmethode reden.

Ok, soll ich dann das Replace raus nehmen?

@Saharel001
Copy link
Contributor Author

War so frei die Backup.sh in dem PR auch anzupassen. Verarbeitung der : ist damit auch raus.

@benderl
Copy link
Contributor

benderl commented Oct 23, 2023

War so frei die Backup.sh in dem PR auch anzupassen. Verarbeitung der : ist damit auch raus.

Das überschneidet sich leider mit #1175, den ich bereits erweitert hatte.
Außerden müsstest Du die Prüfung auf ungültige Zeichen bitte komplett einbauen, anstatt zu entfernen.

@Saharel001
Copy link
Contributor Author

Saharel001 commented Oct 23, 2023

War so frei die Backup.sh in dem PR auch anzupassen. Verarbeitung der : ist damit auch raus.

Das überschneidet sich leider mit #1175, den ich bereits erweitert hatte. Außerden müsstest Du die Prüfung auf ungültige Zeichen bitte komplett einbauen, anstatt zu entfernen.

Konflikt beseitigt.

Habe die Prüfung in der GUI als pattern hinterlegt. openWB/openwb-ui-settings#367

Sollte doch reichen, oder?

@Saharel001
Copy link
Contributor Author

Saharel001 commented Oct 25, 2023

@benderl da kein Feedback gekommen ist, hab ich die Prüfung jetzt noch ins Backend eingebaut. Passt das jetzt?

@Saharel001
Copy link
Contributor Author

Saharel001 commented Oct 25, 2023

Noch eine Ergänzung zu den erlaubten Zeichen (https://learn.microsoft.com/en-us/rest/api/storageservices/naming-and-referencing-shares--directories--files--and-metadata):

The following characters aren't allowed: " \ / : | < > * ?

Demnach sollte nicht nur : ersetzt werden, sondern auch der Pfad auf erlaubte Zeichen geprüft werden und bei ungültigen eine Fehlermeldung ausgegeben werden. Die Eingabe in der GUI sollte ebenfalls durch ein Pattern abgesichert werden.

Hab nur den : entfernt da openwb2 die Sicherung so erstellt. Eventuell gleich bei der Erstellung der Sicherung darauf achten?!

Der Pfad könnte ja auch abseits von der GUI per MQTT gesetzt worden sein und ungültige Zeichen enthalten. Daher muss im Backend immer eine vollständige Prüfung erfolgen.

Wenn ich den Pfad per MQTT im Topic openWB/system/backup_cloud/config als Teil der JSON neu publishe, ist dem Modul das egal. Wenn ich ein Backup ausführe nutzt er den per GUI gesetzten Pfad. Hier werden also die Informationen vom MQTT Broker nicht an die Anwendung übergeben. Habe ich ein spezielles set Topic übersehen?

@Saharel001
Copy link
Contributor Author

erledigt!

Saharel001 and others added 3 commits October 27, 2023 13:13
Co-authored-by: benderl <benderl@users.noreply.github.com>
Co-authored-by: benderl <benderl@users.noreply.github.com>
@benderl benderl added this to the 2.1.2 milestone Nov 7, 2023
@benderl benderl merged commit 9460d97 into openWB:master Nov 7, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ui depends on changes in ui repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants