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

Wrap #authenticate with critical #65

Closed
wants to merge 1 commit into from

Conversation

nevans
Copy link
Contributor

@nevans nevans commented Oct 9, 2023

Is there a reason not to do this? It seems to me that it might be needed to safely handle SASL mechanisms with multiple challenge/response cycles, right?

@nevans nevans force-pushed the wrap-authenticate-with-critical branch from c0921dc to 51b3642 Compare October 15, 2023 10:27
@nevans
Copy link
Contributor Author

nevans commented Oct 21, 2023

Actually, should we do this at all for this method? I didn't really know what critical was doing... I just saw that it was used in many other bits of code both here and in net-pop. Now that I've looked more closely at what it does... it seems unwise for authentication code to fake an OK response.

@tmtm what do you think? Am I missing some context? I'll close this ticket and rebase the others to exclude it. Please let me know if you think #authenticate should handle @error_occurred in some way.

@nevans nevans closed this Oct 21, 2023
@nevans nevans deleted the wrap-authenticate-with-critical branch October 21, 2023 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant