-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Allow domain to be specified for SMB #18473
Conversation
makes sense 👍 |
I agree with the code, but it seems it breaks the windows smb test |
@PVince81 I don't know why that would happen, the Windows SMB tests do not hit the domain logic at all, ever. Hopefully the retest will be all green. Please re-review? cc @icewind1991 |
Well, CI still seems broken. The good news is, since the linked issues are classed as bugs, this can be merged after feature freeze. But it would be nice to get some reviewers cc @icewind1991 @DeepDiver1975 @PVince81 |
Does this need any migration logic to extract the domain part from user names, in case it was added in that field already before ? |
@PVince81 None at all, the domain can still continue to be specified as part of the username, but this approach also works with session credentials where the username cannot be manually set. |
👍 then |
@DeepDiver1975 force push to make retrigger scrutinizer ? |
no push please - not for scrutinizer |
Allow domain to be specified for SMB
👍 @Xenopathic can you do a update at the documentation too? |
@mmattel To be honest, I think with all the changes the documentation for the admin storages GUI will have to be nearly completely rewritten. I'll get to work on it some time this or next week |
This adds an additional field in the SMB storage backend, 'Domain', which when specified is prepended to the username in the format
domain\username
before being handed off to the storage implementation.This was implemented within the backend logic instead of directly in the SMB class, since these sort of pre-mounting parameter mangling is the usecase for the backends, and such logic shouldn't be in the mounting implementation.
Fixes #13407, fixes #17480
cc @icewind1991 @PVince81 @MorrisJobke @DeepDiver1975