-
Notifications
You must be signed in to change notification settings - Fork 15
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
add friendly account name lookup from config, switch to v2 OL API, and some linting #96
Conversation
All unit tests pass now. |
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.
✅ This pull request was sent to the PullRequest network.
@edlitmus you can click here to see the review status or cancel the code review job - or - cancel by adding [!pr] to the title of the pull request.
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.
Aside from some stylistic nits, this code looks good to me. Great job fixing up linter issues. The ones related to error handling can cause issues if they're not taken seriously so it's great to fix them up when you can.
The addition of the authenticated path (rSaml.Message != "Success"
) does make the code deeply nested and slightly difficult to read. I'd recommend refactoring to extract parts of this function into a helper when you have time. This will make the code less error-prone and easier for future readers to wrap their heads around.
I don't have access to the TestGet/Malformed_ARN_components
test mentioned in the comment, so I can't help track down the error you're getting. If you copy-paste that code into a comment I would be happy to help troubleshoot.
Reviewers will be notified any time you reply to their comments or commit new changes.
You can also request a full follow-up review from your PullRequest dashboard.
DoNotNotify: false, | ||
} | ||
|
||
s.Start() |
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.
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.
This will take some refactoring to clean up nicely. I'll certainly take a look when I can.
rMfa, err = c.VerifyFactor(token, &pMfa) | ||
s.Stop() | ||
if err != nil { | ||
return nil, fmt.Errorf("verifying factor: %v", err) |
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.
This point in the code is very deeply nested within if statements, which makes it difficult to tell if the error handling is correct.
Maybe you could extract this into a helper function? Extracting this code into a smaller helper function, maybe something likefetchMFA()
, could reduce the complexity of this code and make it less error-prone.
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.
I'll revisit this when I can to clean it up.
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.
Overall these changes look great. I can see that the intent of this PR was met by parsing the AWS ARN to grab the account name. I have a few minor comments that are mostly centered around future maintainability of these additions, but none of these comments are related to functionality or security concerns and I would not hesitate to merge this PR. Great work!
Reviewers will be notified any time you reply to their comments or commit new changes.
You can also request a full follow-up review from your PullRequest dashboard.
@@ -75,18 +79,29 @@ func extractArns(attrs []saml.Attribute) (arns []ARN) { | |||
} | |||
|
|||
// Prepare patterns | |||
role := regexp.MustCompile(`^arn:aws:iam::\d+:role/\S+$`) | |||
idp := regexp.MustCompile(`^arn:aws:iam::\d+:saml-provider/\S+$`) | |||
role := regexp.MustCompile(`^arn:aws:iam::(?P<Id>\d+):(?P<Name>role\/\S+)$`) |
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.
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.
certainly can be cleaned up at some point
@edlitmus thank you for contributing! I'm sorry for letting it sit so long. I'd be tempted to merge as is and open three issues for the remaining conversations unless you already started obviously. I appreciate it if you would leave a quick note. |
I’m not sure when I can dedicate time for the remaining changes, but if you open some tickets I’m happy to continue working on it. I’d like to get the changes so far merged so I can recommend using this at work. I’ve been using my fork for a while now and feel this is the best user experience by far for saml logins. |
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.
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.
Looks like this was merged while I was going through it; but posting just in case this is useful for you moving forward:
These changes look great. I've gone over this entire review and couldn't find much to comment on, as it is very well written, and Max's detailed comments have been addressed. I left one comment inline that fixes some additional linter-type errors around missing comments for exported items that if addressed would make this PR perfect. Beautifully done!
Reviewers will be notified any time you reply to their comments or commit new changes.
You can also request a full follow-up review from your PullRequest dashboard.
@@ -38,24 +38,18 @@ type GenerateSamlAssertionParams struct { | |||
|
|||
// TODO This one assumes MFA is enabled. Need to handle all cases. |
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.
ISSUE: comments:doc-comments (Severity: Low)
comment on exported type GenerateSamlAssertionResponse should be of the form "GenerateSamlAssertionResponse ..." (with optional leading article)
Remediation:
This type-definition should have a comment that begins with the type name, or otherwise the type name should be decapitalized if it doesn't need to be exported.
The same applies to the three other struct
type-definitions below: VerifyFactorParams
, VerifyFactorResponse
, and GetUserByEmailResponse
.
The same further applies to the ARN
type-definition and the Get
function in saml/saml.go
.
Related issue: #95
This tries to look up human friendly account names from
global:accounts
in the config file and match them to the listed account ARN.My editor has auto-linting turned on and caught some issues, so I fixed them as well.
Go modules updated as well.
All unit tests pass.
Switched to the newer v2 API for OneLogin which fixes the errors seen in #85