-
Notifications
You must be signed in to change notification settings - Fork 296
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
Feature/auth by pam #149
Feature/auth by pam #149
Conversation
… into feature/auth-by-pam
def listdir(root): | ||
return listdir_cache.get(root, _listdir) | ||
except ImportError: | ||
listdir = _listdir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blade2005 what's the purpose of this block in relation to the pam auth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ha. I know what happened. This is older code from my version of core.py that got patched. When I rebased it got pulled back in.
See this is why you shouldn't game and code.
I'll refactor and redo the PR. Let's just get this version out for now.
EDIT: What I mean is that I'll close this PR and we can release without the pam auth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup as I thought. This section was removed in 18dc199 and because I was hurrying I didn't look closely enough at what git was telling me.
Bueno! |
So, not to be persnickety, but I would like to get some tests in for this if possible. I can work on adding them, but I won't have time before I leave tomorrow for vacation. As such, I think we can wait and push this out as an update to the next version, unless you have any objections, @ankostis. |
Meh, looking at it, we're not really doing any logic. It would be the equivalent of testing pam's code, which I don't think we need to do. I'm going to merge this and release a new beta. |
@blade2005 can you confirm that the most recent version |
Current master: 0bf0024 works as expected. I test with a incorrect password and got a 403 forbidden response. Cleared cached and tried again and was able to download the package. Test pypi-server with API accessors used was
|
@blade2005 I cannot use this auther to upload a package. Also for the example code to work, the following dependency is needed (which is missing from the docs): |
@mplanchard said:
Yes, I agree. |
Also I suggest to squash small commits like these. |
Because the import is inside the function rather than at the top it is imported only in the context of the function itself causing it to be an optional dependancy. Though the README should be updated to reflect that. My original plan had been to squash it all into one. I usually do. |
@ankostis, with a quick update to the README, I think we're good to release. I can get that done today if you don't have the time. Let me know. |
using the file i provided above and running that in a vagrant instance.
Which in the end gave me the following:
Upload does work. I will do another PR to reflect that. I will also add some better error handling for core.py on the import if someone doesn't have pam installed. |
|
@ankostis you're right. I haven't packaged the standalone script before. Seems like we run |
Feature/auth by pam
No description provided.