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

Adds possibility to change another user password #42

Closed
wants to merge 1 commit into from
Closed

Adds possibility to change another user password #42

wants to merge 1 commit into from

Conversation

GuillaumeGomez
Copy link
Contributor

I sent the patch to contributing@samba.org already, but I'm not sure it's enough, so I post it here as well.

@abartlet
Copy link
Member

abartlet commented Dec 4, 2015

Thanks. This (and samba-technical) is where we discuss patches, so you have indeed done the right thing.

In terms of the actual patch, can you investigate if the same kind of modification could be made to samba-tool or net instead? smbpasswd is essentially frozen in time at this point, new functionality is added to our more general (and esaiser to expand) tools.

In particular, this is because smbpasswd has enough of a non-standard command-line syntax as it is (inconsistent with the rest of samba, and getopt rather than popt based).

Sorry,

Andrew Bartlett

@GuillaumeGomez
Copy link
Contributor Author

The change is only on smbpasswd and is what I need. I didn't change any "out" function, so I guess they already have the correct prototype, but wasn't used correctly in smbpasswd. So that's why I changed it this way.

@GuillaumeGomez
Copy link
Contributor Author

So what will happen now ? I only updated smbpassd source code because other used libs' functions already fit what I needed. I don't see how I could do it differently...

@abartlet
Copy link
Member

abartlet commented Dec 9, 2015

I understand it was the easiest to change, but we sadly can't accept such a patch to smbpasswd.

The easiest patch to write is sadly often not the best patch for the project as a whole.

I would suggest finding a way to patch 'samba-tool user password' to cover the auth/change user split you are after. It is a combination of python and C, but perhaps you can patch py_net_change_password in python/samba/netcmd/py_net.c and python/samba/netcmd/user.py to have such an option.

@GuillaumeGomez
Copy link
Contributor Author

I don't get it. Why should I update samba-tool when I'm using smbpasswd ? Is samba-tool going to replace smbpasswd ?

@GuillaumeGomez
Copy link
Contributor Author

Ok, as I understood it, samba-tool is the replacement of smbpasswd (or at least, does what smbpasswd did), so I'll update my patch in consequence.

@GuillaumeGomez
Copy link
Contributor Author

(Happy new year !). I added the change on smbpasswd as well. So now, each version is in its own commit.

@GuillaumeGomez
Copy link
Contributor Author

Any news on this pull request?

@aaptel
Copy link
Contributor

aaptel commented Jan 19, 2016

Additionally, since the logon name has a maximum length your patch could be simplified.

From https://technet.microsoft.com/it-it/library/bb726984%28en-us%29.aspx:

Logon names can be up to 104 characters. However, it isn't practical to use logon names that are longer than 64 characters.

@GuillaumeGomez
Copy link
Contributor Author

I wanted to make the change as little as possible but that seems to be a good idea. Thanks @aaptel!

@abartlet
Copy link
Member

On Mon, 2016-01-18 at 02:20 -0800, Guillaume Gomez wrote:

Any news on this pull request?

Reply to this email directly or view it on GitHub.

This pull request isn't going anywhere, sorry.

I know it isn't simple to patch our other tools, but smbpasswd is not
going to get another option, even one as 'simple' as this. It is
legacy tool kept because it is often used and may be referenced in
scripts. The new tools are 'net' and 'samba-tool'.

Sorry,

Andrew Bartlett

Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org
Samba Developer, Catalyst IT http://catalyst.net.nz/services/samba

@GuillaumeGomez
Copy link
Contributor Author

@abartlet: That's what I did in the second commit, I updated 'samba-tool'. If you want, I can remove the first one from the logs to allow you to take a simpler look.

@samba-team-bot
Copy link

On Mon, 2016-01-25 at 02:37 -0800, Guillaume Gomez wrote:

@abartlet: That's what I did in the second commit, I updated 'samba
-tool'. If you want, I can remove the first one from the logs to
allow you to take a simpler look.

Thanks. Patches to Samba need to be a series of 'perfect' commits - we
don't merge your development history, only the final result.

Andrew Bartlett

Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org
Samba Developer, Catalyst IT http://catalyst.net.nz/services/samba

@ghost
Copy link

ghost commented Jan 25, 2016

Can somebody explain why the code in source4 needs to be altered when samba-tool already does what GuillaumeGomez is trying to do

i.e.
samba-tool user setpassword --help
Usage: samba-tool user setpassword (|--filter ) [options]

Set or reset the password of a user account.

The command may be run from the root userid or another authorized userid. The
-H or --URL= option can be used to execute the command against a remote
server.

I am not adverse to changes if they are required and helpful, but are they needed here ?

@GuillaumeGomez
Copy link
Contributor Author

I kept the first one to allow to easily understand what the second does (the version 4 of samba source code is very difficult to read and understand). Anyway, I removed the first commit, it only remains the source 4 commit.

@GuillaumeGomez
Copy link
Contributor Author

@hortimech: Hum... Is it really the same thing? I took a look and didn't find any "obvious" way to do the thing I was looking for (and as far as I know, it wasn't possible in version 3, and that's the original reason of this PR).

@ghost
Copy link

ghost commented Jan 25, 2016

As far as I am aware, the code in source4 is for Samba 4.x.x and samba-tool only works with Samba 4.x.x. I think you are also missing the fact that Samba 3.x.x is EOL.

I have no idea if your code is good code or otherwise, I just don't want something changing for a corner case if this may adversely affect others.

@GuillaumeGomez
Copy link
Contributor Author

Well, it's your call. Just keep me up to date about your final decision please.

@bjacke
Copy link
Contributor

bjacke commented May 31, 2018

you are right, that samba-tool is not currently the tool useful on Samba servers in "classic" mode, so your request for enhancing with a patch was not wrong. But I think the functionality is there already: as root you can just run "smbpasswd foo" and as a normal user you can change someone else's password with "smbpasswd -U foo".

https://bugzilla.samba.org/ is the tool you shuld use to report bugs and feature requests, please use that next time.

Thanks
Björn

@bjacke
Copy link
Contributor

bjacke commented May 31, 2018

you're right, that samba-tool is not yet very useful for Samba servers in "classic" mode and your request to have that feature in smbpasswd is quite valid. But the feature is already implementen as far as I can see: as root you can run "smbpasswd foo" to set someone's password and as a user you can set someone else's password with "smbpasswd -U foo", can't you?

https://bugzilla.samba.org/ is the right way to report bugs and feature requests for Samba, please use that in the future.

Thanks
Björn

@bjacke bjacke closed this Jun 1, 2018
mattiasa pushed a commit to mattiasa/samba that referenced this pull request Oct 29, 2018
Set "inherited" flag when winmsa changes ACL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants