Provide better error message when API authentication fails #5683
Conversation
labkey-jeckels
left a comment
labkey-jeckels
left a comment
There was a problem hiding this comment.
Changes look OK but I suspect the test failures in the BVTs are related.
|
|
||
| return null; | ||
| // Basic auth has failed so send failure response in a format that APIs can consume | ||
| // TODO: Shouldn't our client APIs set a default "respFormat" attribute? Java library doesn't... |
There was a problem hiding this comment.
I added respFormat years ago for a niche use case that could only consume XML. I think it's fine to default to JSON and not bother explicitly requesting it.
There was a problem hiding this comment.
This was necessary to get ClientAPITest to pass, since it sets "respFormat" to XML in a few cases, and was unhappy with hard-coded JSON response. Happy to discuss and clean this up on develop or later 24.7.x... I don't love what feels like one-off handling here.
labkey-jeckels
left a comment
labkey-jeckels
left a comment
There was a problem hiding this comment.
Please hold off on merging until we have a decision on timing. See my question to the Release Management group in the chat.
Rationale
We want API clients to receive more helpful guidance when authentication fails. For example, password expired vs. password doesn't meet complexity requirements vs. bad credentials. https://www.labkey.org/home/Developer/issues/issues-details.view?issueId=50841
Changes
AuthenticationStatuscan now tailor error messages for web UI vs. API purposes. For example, returning a message to a piece of code that suggests checking the caps lock seems unhelpful.