Authenticate native and pam_password with iRODS 4.3+ auth framework

[d-w-moore]

No description provided.

Looks good. Have a good number of questions since I'm not that familiar with more complex Python, and a few on code style.

Looks pretty good to me! Just a few minor things.

I still see 5 unresolved comments. Is that intentional?

Good stuff so far.

We'll need to update the README to cover various things once the code changes are done. The item that comes to mind is the pool option that disables authentication.

Force pushed to repartition the two commits with regard to specific code diffs that were mis-categorized. After the rejigger, branches still compared the same.

I still see 5 unresolved comments. Is that intentional?

With me it's often that I'm not confident the question or subissue has been sufficiently cleared up, and I've not received an answer.

Having said that, I think I have now resolved all that I can, comfortably. (Thinking of both my comfort and that of the person who posted the question / comment). : )

Ok. I'll go through your responses shortly.

New plugins looks great. They have roughly similar shapes to the C++ so I was able to follow them pretty easily.

Please run Black formatter on these changes.

Also, I am morally obligated to ask if there is a need for any new auth tests given the changes, but I suppose if we run the tests against a 4.2.x server and a 4.3.x server and everything works, that's sufficient. It would probably need to be part of the standard tests before a release to prevent regressions until we drop support for things before 4.3.

Perhaps we should add some test configuration to allow for using the "force legacy auth" option so we make sure everything is still working. Please feel free to punt this to a separate issue in the same release milestone as I wouldn't consider that a blocker for this PR.

New plugins looks great. They have roughly similar shapes to the C++ so I was able to follow them pretty easily.

Please run Black formatter on these changes.

Ok, will do when things are final. Leaving this convo unresolved til then.

Perhaps we should add some test configuration to allow for using the "force legacy auth" option so we make sure everything is still working. Please feel free to punt this to a separate issue in the same release milestone as I wouldn't consider that a blocker for this PR.

Should be simple enough to at least add a test that force_legacy_auth works as advertised, i.e. pushes us into the right codepath and results in a successful native login.

Also, I am morally obligated to ask if there is a need for any new auth tests given the changes, but I suppose if we run the tests against a 4.2.x server and a 4.3.x server and everything works, that's sufficient. It would probably need to be part of the standard tests before a release to prevent regressions until we drop support for things before 4.3.

Just so. Test suite seems to pass with changes so far.

Perhaps we should add some test configuration to allow for using the "force legacy auth" option so we make sure everything is still working. Please feel free to punt this to a separate issue in the same release milestone as I wouldn't consider that a blocker for this PR.

I've pushed a new test to the PR to ensure that at least the force_legacy_auth setting does as advertised for the native auth case, so @alanking , please eye it and let me know please if you think more is needed.

Looks good overall. Had one concern and two tweaks. I think this is close...

Getting there.

What's left for this PR?

Good. Let us know when it's ready for a final review.

Right now debugging some failing test scripts - other than that though , ready for final review.

Ok. We'll put some eyes on this soon.

More insignificant observations

Status check... What's left to do for this? Are the test scripts working now?

Status check... What's left to do for this? Are the test scripts working now?

As far as I know. I've done a few (very small) changes since those tests were run, so ... would like to run all the tests again today. Then maybe a merge party tomorrow?

Sounds good to me. Thanks

Looks like the commits have been squashed.

Seems the body of each commit message is in an intermediate state?

Looks like the commits have been squashed.

Seems the body of each commit message is in an intermediate state?

That's been corrected

All tests are passing, except for test6 in the case of iRODS server version < 4.3

Feel free to start final review , especially of the README changes which are really the main thing that's new.

Just a few suggestions on the README. Looks good.

As for the failing test, would it be worth opening a separate issue to address that so that we can get this one wrapped up? "No, let's fix it now" is a perfectly good answer.

Good stuff.

I think we're ready for pounds.