Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Css 6646/callback endpoint #1170
Css 6646/callback endpoint #1170
Changes from 7 commits
e7f6a06
b9818e4
d36300f
08677e0
93c6430
a828285
b983288
f7c19f9
86e1093
c069bca
ba98f51
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
some validation of input parameters should be done here, no? like.. store should not be nil, sessionTokenExpiry should not be 0.. i'm sure there's other validation we could do
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 feel like validation should be done at the service.go level, but happy to add more in?
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.
is this unit tested?
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.
also.. it doesn't make sense to pass in email as a separate parameter.. you could just call
here to get the email from the token
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.
It isn't tested individually it's part of the TestDevice, I need to come back and mock all of this out afterwards. That's the plan anyways, so I guess view the tests here as temporary, I just written them to help develop. And it's separate as I think the extracting email from a field that can error out is better done outside of the function? Happy to call .Email() within updateidentity but I feel this is clearer / more obvious?
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.
also in next pr we actually use the email in another function within the handler, so i'll leave this as is
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.
why??
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.
We pass it over to the session creation
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.
Should we create a ticket for this and link it here?
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 think one might exist... i am a bit confused by it though. trhis was more babaks suggestion
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.
Email does not need to be a method of the authenticator.. it can be an independent function because it just extracts an email from the id token that's passed in
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.
Answered in the other comment why I did this
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 is good enough, but I think we should give it another thought. All the tests are created with "no dashboard required for this test", which could have been avoided if this was not required here as we return an error.
Moreover, checking on empty strings shouldn't be enough, we need to actually verify the url is parseable, valid, and reachable. If not, return an error, too
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 is because the auth handler has validation, which I agree with Ales it was necessary, just our other handlers don't have the same validation... Because we embed into jimm a lot we end up with this polluted mess of creating stuff for the sake of creating it. And ah yeah definitely, I'll add a url parse to next PR totally agree.