Skip to content

Authenticate native and pam_password with iRODS 4.3+ auth framework #685

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 19, 2025
Merged

Authenticate native and pam_password with iRODS 4.3+ auth framework #685

merged 3 commits into from
Mar 19, 2025

Conversation

d-w-moore
Copy link
Collaborator

No description provided.

@d-w-moore d-w-moore marked this pull request as draft January 28, 2025 20:18
@d-w-moore d-w-moore self-assigned this Jan 29, 2025
MartinFlores751
MartinFlores751 reviewed Jan 29, 2025
View reviewed changes
Copy link
Contributor

@MartinFlores751 MartinFlores751 left a comment

Choose a reason for hiding this comment

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

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

FifthPotato
FifthPotato reviewed Jan 31, 2025
View reviewed changes
Copy link

@FifthPotato FifthPotato left a comment

Choose a reason for hiding this comment

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

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

@d-w-moore d-w-moore force-pushed the conn43.pr branch 4 times, most recently from 75cb130 to 71fcf98 Compare February 14, 2025 17:19
@d-w-moore d-w-moore marked this pull request as ready for review February 14, 2025 17:30
@korydraughn
Copy link
Contributor

korydraughn commented Feb 14, 2025 •
edited
Loading

I still see 5 unresolved comments. Is that intentional?

korydraughn
korydraughn reviewed Feb 14, 2025
View reviewed changes
Copy link
Contributor

@korydraughn korydraughn left a comment

Choose a reason for hiding this comment

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

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.

@d-w-moore
Copy link
Collaborator Author

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.

@d-w-moore
Copy link
Collaborator Author

d-w-moore commented Feb 17, 2025 •
edited
Loading

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). : )

@korydraughn
Copy link
Contributor

Ok. I'll go through your responses shortly.

alanking
alanking reviewed Feb 17, 2025
View reviewed changes
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

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.

@d-w-moore
Copy link
Collaborator Author

d-w-moore commented Feb 19, 2025 •
edited
Loading

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.

@d-w-moore d-w-moore force-pushed the conn43.pr branch 2 times, most recently from 7a344ee to 6569e55 Compare February 20, 2025 15:09
@d-w-moore
Copy link
Collaborator Author

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.

alanking
alanking reviewed Feb 20, 2025
View reviewed changes
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

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

korydraughn
korydraughn reviewed Feb 21, 2025
View reviewed changes
korydraughn
korydraughn reviewed Feb 25, 2025
View reviewed changes
Copy link
Contributor

@korydraughn korydraughn left a comment

Choose a reason for hiding this comment

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

Getting there.

What's left for this PR?

@d-w-moore d-w-moore marked this pull request as draft March 4, 2025 11:56
@korydraughn
Copy link
Contributor

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

@d-w-moore
Copy link
Collaborator Author

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

@korydraughn
Copy link
Contributor

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

alanking
alanking reviewed Mar 14, 2025
View reviewed changes
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

More insignificant observations

@alanking
Copy link
Contributor

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

@d-w-moore
Copy link
Collaborator Author

d-w-moore commented Mar 17, 2025 •
edited
Loading

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?

@alanking
Copy link
Contributor

Sounds good to me. Thanks

@korydraughn
Copy link
Contributor

Looks like the commits have been squashed.

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

@d-w-moore d-w-moore force-pushed the conn43.pr branch 2 times, most recently from 4b5a557 to ea679b8 Compare March 19, 2025 05:47
@d-w-moore
Copy link
Collaborator Author

Looks like the commits have been squashed.

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

That's been corrected

@d-w-moore
Copy link
Collaborator Author

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.

alanking
alanking reviewed Mar 19, 2025
View reviewed changes
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

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.

korydraughn
korydraughn reviewed Mar 19, 2025
View reviewed changes
korydraughn
korydraughn reviewed Mar 19, 2025
View reviewed changes
@alanking alanking mentioned this pull request Mar 19, 2025
Open
korydraughn
korydraughn reviewed Mar 19, 2025
View reviewed changes
korydraughn
korydraughn approved these changes Mar 19, 2025
View reviewed changes
Copy link
Contributor

@korydraughn korydraughn left a comment

Choose a reason for hiding this comment

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

Good stuff.

I think we're ready for pounds.

@d-w-moore
[irods#686] independent computation of irodsA path
0971f3d
@d-w-moore
[irods#688] implement server_version_without_auth
53405a8 
This is a method of the iRODSSession object that returns the connected iRODS server
version without having to authenticate first.
@d-w-moore
[irods#499] authenticate native and pam_password schemes using iRODS …
2f30578 
…4.3+ auth flow
@alanking alanking merged commit bee8294 into irods:main Mar 19, 2025
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trel trel trel left review comments

@korydraughn korydraughn korydraughn approved these changes

@alanking alanking alanking approved these changes

@MartinFlores751 MartinFlores751 Awaiting requested review from MartinFlores751

@FifthPotato FifthPotato Awaiting requested review from FifthPotato

Assignees

@d-w-moore d-w-moore

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@d-w-moore @korydraughn @alanking @trel @MartinFlores751 @FifthPotato