-
Notifications
You must be signed in to change notification settings - Fork 177
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
🎨 Clearer app sign on rule failure messages #180
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Invert condition and blocks to put failure case last - Add comments about heuristics for re-auth and app-level MFA
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,20 +68,24 @@ private String getSamlResponseForAwsFromDocument(Document document) { | |
} else { | ||
Elements errorContent = document.getElementsByClass("error-content"); | ||
Elements errorHeadline = errorContent.select("h1"); | ||
if (errorHeadline.isEmpty()) { | ||
throw new RuntimeException("You do not have access to AWS through Okta. \nPlease contact your administrator."); | ||
} else { | ||
if (errorHeadline.hasText()) { | ||
throw new RuntimeException(errorHeadline.text()); | ||
} else { | ||
throw new RuntimeException("You do not have access to AWS through Okta. \nPlease contact your administrator."); | ||
} | ||
} | ||
} | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ‘’missed if” There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’ll fix that and merge. Thank you for code reviews. |
||
private boolean isPasswordAuthenticationChallenge(Document document) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be helpful if we could point to the API docs where these strings are defined? |
||
return document.getElementById("password-verification-challenge") != null; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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? |
||
|
||
// Heuristic based on undocumented behavior observed experimentally | ||
// This condition may be missed if Okta significantly changes the app-level MFA page | ||
private boolean isPromptForFactorChallenge(Document document) { | ||
return document.getElementById("okta-sign-in") != null; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@randomsamples this is much cleaner with your suggestions. Thank you 🎉