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

Make pam.d directory configurable #2552

Merged
merged 1 commit into from
Feb 21, 2023
Merged

Conversation

yifanjiang
Copy link
Contributor

Change according to the discussion in #2547 .

@yifanjiang yifanjiang marked this pull request as ready for review February 15, 2023 03:04
@matt335672
Copy link
Member

Hi @yifanjiang

I'm out of time this week to have a good look at this, but after a quick glance this indeed looks like I was thinking of. This will definitely be useful to SuSE and any other distros that don't use /etc/pam.d.

A quick question- is there a good reason why you didn't add /pam.d to the pam config dir?

Speak soon.

@yifanjiang
Copy link
Contributor Author

A quick question- is there a good reason why you didn't add /pam.d to the pam config dir?

Hi @matt335672 , thanks for looking into this. The reason was to make it a little more flexible for downstream packaging. While I took a second look, keeping a consistent lexical style with socketdir and sesmanruntimedir might be a better choice. So I update through the changes.

Copy link
Member

@metalefty metalefty left a comment

Choose a reason for hiding this comment

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

I'm fine with this now. Waiting for @matt335672 's approval.

configure.ac Outdated Show resolved Hide resolved
Copy link
Member

@matt335672 matt335672 left a comment

Choose a reason for hiding this comment

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

I was all ready to approve this, then I read this useful comment from @thkukuk

I suggest the following amendment.

@yifanjiang - I apologise for messing you about like this. However, what we've now got something that should be able to support the use-cases mentioned by @thkuk

sesman/verify_user_pam.c Show resolved Hide resolved
@matt335672
Copy link
Member

Thanks for the quick edits @yifanjiang

Again, I apologise for the confusion.

@matt335672
Copy link
Member

@metalefty - I think this should be backported to v0.9. Do you agree?

@metalefty
Copy link
Member

@matt335672 Yes, it is definitely useful.

@metalefty metalefty added this to the v0.9.22 milestone Feb 27, 2023
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.

3 participants