🎨 Clearer app sign on rule failure messages #180
Conversation
- Differentiate sign-on failure due to app-level MFA - Differentiate sign-on failure due to app-level re-auth challenge - Use failure message from Okta when available otherwise
|
@randomsamples I was inspired by your work in #172 to take this a little further. Can you give this a code review for me? |
| } else { | ||
| Elements errorContent = document.getElementsByClass("error-content"); | ||
| Elements errorHeadline = errorContent.select("h1"); | ||
| if (errorHeadline.isEmpty()) { |
There was a problem hiding this comment.
I would invert this conditional so that the failure case (no error-content nodes) is last in the code.
There was a problem hiding this comment.
Good catch! I will do that. Thank you.
| } | ||
| return samlResponseInputElement.attr("value"); | ||
| } | ||
|
|
||
| private boolean isPasswordAuthenticationChallenge(Document document) { |
There was a problem hiding this comment.
It might be helpful if we could point to the API docs where these strings are defined?
- Invert condition and blocks to put failure case last - Add comments about heuristics for re-auth and app-level MFA
| // This condition may be missed Okta significantly changes the app-level re-auth page | ||
| private boolean isPasswordAuthenticationChallenge(Document document) { | ||
| return document.getElementById("password-verification-challenge") != null; | ||
| } |
There was a problem hiding this comment.
@randomsamples hopefully the comments on these methods make up for the lack of documentation. The risk is mostly that the tool will incorrectly say that the user doesn't have app access if Okta changes these pages (the current behavior prior to this change). Is that an acceptable compromise?
| throw new RuntimeException(errorHeadline.text()); | ||
| } else { | ||
| throw new RuntimeException("You do not have access to AWS through Okta. \nPlease contact your administrator."); | ||
| } |
There was a problem hiding this comment.
@randomsamples this is much cleaner with your suggestions. Thank you 🎉
|
@randomsamples can you give this one last once over to see that I've addressed your concerns? |
| } | ||
| return samlResponseInputElement.attr("value"); | ||
| } | ||
|
|
||
| // Heuristic based on undocumented behavior observed experimentally | ||
| // This condition may be missed Okta significantly changes the app-level re-auth page |
There was a problem hiding this comment.
I’ll fix that and merge. Thank you for code reviews.
Problem Statement
When app sign on policies are unsupported the tool responds like this:
That is inaccurate and confusing and leads to issue reports that would be better handled as feature requests with use cases outlined.
Solution
Differentiate sign-on failure due to app-level MFA
Differentiate sign-on failure due to app-level re-auth challenge
Use failure message from Okta when available otherwise