Alternative SMB external storage implementation where all users of on… #38151
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
DeepDiver1975
force-pushed
the
feature/shared-cache-for-external-storages-2
branch
from
57248b0
to
52c8f00
Compare
DeepDiver1975
force-pushed
the
feature/shared-cache-for-external-storages-2
branch
from
77d937f
to
0ff8c57
Compare
| @@ -117,7 +117,8 @@ public function jsonSerialize() { | |||
| public function validateStorage(IStorageConfig $storage) { | |||
| // does the backend actually support this scheme | |||
| $supportedSchemes = $storage->getBackend()->getAuthSchemes(); | |||
| if (!isset($supportedSchemes[$this->getScheme()])) { | |||
| if (!isset($supportedSchemes[$this->getScheme()]) && | |||
| !isset($supportedSchemes[$this->getIdentifier()])) { | |||
There was a problem hiding this comment.
Not sure if this is a good idea. Having an auth mechanism id instead of a scheme id will be confusing.
It might be better to keep using the scheme and allow username+password even though it doesn't make sense for this scenario. Note that WND will add new auth mechanism that might be usable in this context too.
There was a problem hiding this comment.
I honestly want to limit this storage to sessioncredentials (and in wnd whatever will work there) anything else will just result in bullshit.
But yes - i am not too happy with this either - maybe we better introduce an explicit array of auth identifiers in addition ....
| $this | ||
| ->setIdentifier('smb2') | ||
| ->setStorageClass('\OCA\Files_External\Lib\Storage\SMB2') | ||
| ->setText($l->t('SMB / CIFS (version 2)')) |
There was a problem hiding this comment.
This might need some rewording:
- Is it for SMB2 protocol? Can I use this with a SMB1 connection?
- Does this new code deprecated the previous SMB mounts? (version 2 suggests improvements over the old SMB connection code)
As far as I know, previous SMB code is still valid and might perform better in some scenarios, so it seems this implementation can live side by side with the old one.
There was a problem hiding this comment.
Yes - this is where product management is called to do some naming bingo ;-)
The 'old' implementation is still valid to be used ... no need fro deprication
|
Note sure if we need to include a cache for the stat calls. Other than the code, the code looks fine |
We will find out in some real life testing .... |
|
I would name this: "shared file ID" in German: "file ID Mehrfachnutzung" maybe @pmaier has a better idea ... |
|
Regarding wording, I actually like @DeepDiver1975's suggestion in another channel:
|
|
converting back to draft because of detected side effects while testing .... |
DeepDiver1975
force-pushed
the
feature/shared-cache-for-external-storages-2
branch
2 times, most recently
from
8fc1dea
to
d974ec0
Compare
DeepDiver1975
force-pushed
the
feature/shared-cache-for-external-storages-2
branch
from
d974ec0
to
a8c13fd
Compare
…e share use the same storage id. User specific access is filtered in a cache wrapper by stating the files on SMB Only password::sessioncredentials is allowed for authentication and SMBv2 can only be setup by admins
DeepDiver1975
force-pushed
the
feature/shared-cache-for-external-storages-2
branch
from
a8c13fd
to
941d393
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
delete-merged-branch
bot
deleted the
feature/shared-cache-for-external-storages-2
branch
…e share use the same storage id. User specific access is filtered in a cache wrapper by stating the files on SMB
Description
How Has This Been Tested?
Screenshots (if appropriate):
Setup of SMB version 2
Same file id is used and as result comments and tags are show properly on both users:
File ID is the same - see last query parameter in url
Tow users with different permissions see folder content accordingly:
Types of changes
Checklist: