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

Fix joining with samba 4.21 #122

Merged
merged 2 commits into from
Oct 21, 2024
Merged

Fix joining with samba 4.21 #122

merged 2 commits into from
Oct 21, 2024

Conversation

scabrero
Copy link
Collaborator

Problem

Samba 4.21 changes the way the system keytab is created. To add the machine account name to the keytab it is necessary to set sync machine password to keytab = /etc/krb5.keytab:account_name:sync_etypes:sync_kvno:machine_password in /etc/smb.conf.

Solution

Detect the installed samba version and add the necessary parameter.

Testing

Manually tested.

@coveralls
Copy link

coveralls commented Sep 26, 2024

Pull Request Test Coverage Report for Build 11363037220

Details

  • 15 of 32 (46.88%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 25.198%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/lib/auth/authconf.rb 14 31 45.16%
Totals Coverage Status
Change from base Build 10845491006: 0.4%
Covered Lines: 478
Relevant Lines: 1897

💛 - Coveralls

Copy link

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

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

Sorry for being late with the review. We have been busy with other stuff.

The code looks correct, although I have suggested some improvements. Basically, we should stop using the old Yast::Builtins and similar methods. As long as possible, please let's use ruby std classes and methods instead.

I see there already is a test/authconf_test.rb file. Could be possible to add some tests for these changes there?

Thanks!

@scabrero
Copy link
Collaborator Author

Sorry for being late with the review. We have been busy with other stuff.

The code looks correct, although I have suggested some improvements. Basically, we should stop using the old Yast::Builtins and similar methods. As long as possible, please let's use ruby std classes and methods instead.

I see there already is a test/authconf_test.rb file. Could be possible to add some tests for these changes there?

Thanks!

Thanks for the review, this looks much better now. I have added a test for the version check.

Signed-off-by: Samuel Cabrero <scabrero@suse.de>
Copy link

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

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

Version bump is missing in package/yast2-auth-client.spec.

src/lib/auth/authconf.rb Outdated Show resolved Hide resolved
src/lib/auth/authconf.rb Outdated Show resolved Hide resolved
Signed-off-by: Samuel Cabrero <scabrero@suse.de>
@joseivanlopez
Copy link

I see you marked as solved my last 2 comments, but I see no changes in the code. And the spec file is not updated neither with the new version.

@scabrero
Copy link
Collaborator Author

I see you marked as solved my last 2 comments, but I see no changes in the code. And the spec file is not updated neither with the new version.

Sorry, I wanted to test the changes before pushing and some tasks got in the middle.

Copy link

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

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

Sorry @scabrero, I was on vacation when the review was requested. Fortunately, @joseivanlopez took care and changes looks good to him. So, I guess you can hit the merge button 🎉

@scabrero scabrero merged commit b51ba27 into yast:master Oct 21, 2024
4 checks passed
Copy link

❌ Autosubmission job #11437603416 failed

dgdavid added a commit that referenced this pull request Oct 22, 2024
Missing in #122, not
autosubmitted because

> Stopping, missing new bugzilla or fate entry in the *.changes file.
>
> https://github.com/yast/yast-auth-client/actions/runs/11437603416/job/31817441511
@dgdavid
Copy link
Member

dgdavid commented Oct 22, 2024

Auto-submission sent successfully after adding a reference in the latest changelog entry, see #123 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants