Add the #remove_share method to the server #260
Conversation
zeroSteiner
force-pushed
the
feat/server/remove-share
branch
from
4be984f to
feba8f6
Compare
lib/ruby_smb/server.rb
Outdated
| @@ -58,6 +58,14 @@ def add_share(share_provider) | |||
| @shares[share_provider.name] = share_provider | |||
| end | |||
|
|
|||
| def remove_share(share_provider) | |||
| share_provider = share_provder.name if share_provider.is_a?(RubySMB::Server::Share::Provider::Base) | |||
There was a problem hiding this comment.
Two things.....
One, this looks like a typo?
| share_provider = share_provder.name if share_provider.is_a?(RubySMB::Server::Share::Provider::Base) | |
| share_provider = share_provider.name if share_provider.is_a?(RubySMB::Server::Share::Provider::Base) |
Two, this looks like we're taking an element of an object and overwriting that object with the object's element?
On second thought, I see. I'm guessing that @shares.delete() takes a string that is the share_provider.name value, but this line will overwrite the parameter to the share_provider.name value if the user passes in the share_provider object.
Could we maybe add a different variable name for this like share_provider_name, or even share_provider_to_delete? Maybe even just a comment that this can take either the name or the share_provider object?
There was a problem hiding this comment.
- Yes, that's a typo and I'll fix it.
- I'm confused as to what you're saying. Basically
share_providerneeds to be the share name to remove. This line only serves to obtain that value from the#nameattribute if theshare_provideris an instance ofRubySMB::Server::Share::Provider::Base, otherwise it needs to be a string. I'll add some docs to clarify that.
There was a problem hiding this comment.
Per #2- It just struck me as odd we were renaming an object to an objects attribute. I think the crux was that you're leveraging weak typing here as a feature to create a method that is designed to take multiple parameter types and deal with the properly, but even after years of Ruby and Python, it came as a surprise to me. I think just a quick one-liner to explain that was your goal, or to have something documenting the multitype support for the method definition.
There was a problem hiding this comment.
I added some YARD docs for the parameter that call out it accepts either a String or Share::Provider::Base instance. Let me know if you think I can make it more clear.
Release NotesThis PR adds the |
This pulls in the changes from rapid7/ruby_smb#260 which adds the #remove_share method that is needed for cleanup.
This is an easy one. It just adds the
#remove_sharemethod to the server. I'll need to use it to clean up once a single server instance can be shared by multiple modules in Metasploit as an HTTP service can be.For testing, the easiest thing would probably be to wait until the corresponding Metasploit PR has been made and then just check that the cleanup method used it to remove the share using
smbclient(e.g.smbclient //192.168.159.128/Share1 -U smcintyre -N -c "dir *").