Skip to content
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

3645 sync user during login #5077

Merged

Conversation

kberkos-public
Copy link
Contributor

No description provided.

@LibbyBot
Copy link

Code analysis and actions

DO NOT DELETE THIS COMMENT.
  • 3 product code files were changed.
  • Please describe in a separate comment how you tested your changes.

@kberkos-public kberkos-public added release bug This bug is present in a released version of Open Liberty and removed CLA Signed labels Sep 13, 2018
@kberkos-public kberkos-public self-assigned this Sep 13, 2018
@kberkos-public
Copy link
Contributor Author

kberkos-public commented Sep 13, 2018

#3645

Copy link
Contributor

@kaczyns kaczyns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kyle - thanks for making these changes. I'd like Teddy to take a look at this and be the approver. It all looks fine to me, but I am not as familiar with the overall security code and I want to make sure that these changes are not going to break some other part of the security code. I'd also like to talk with you about the FAT tests since those would be delivered separately.

@@ -496,6 +505,17 @@ private void setSubjectAndCookies(HttpServletRequest req, HttpServletResponse re
if (addSSOCookie) {
ssoCookieHelper.addSSOCookiesToResponse(subject, req, resp);
}
try {
Object loginToken = ThreadIdentityManager.setAppThreadIdentity(subject);
WebSecurityContext webSecurityContext = (WebSecurityContext) SRTServletRequestUtils.getPrivateAttribute(req, "SECURITY_CONTEXT");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems there should be a constant here for the "SECURITY_CONTEXT" string

@@ -567,8 +576,10 @@ public Object preInvoke(HttpServletRequest req, HttpServletResponse resp, String
performSecurityChecks(req, resp, receivedSubject, webSecurityContext);
}

extraAuditData.put("HTTP_SERVLET_REQUEST", req);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering why the extra audit data was deleted as this doesn't seem to have anything to do with the sync to thread changes, but then again I don't know much about these parts so maybe it was necessary =)

Copy link
Contributor

@teddyjtorres teddyjtorres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good. Please address comments.

@kberkos-public
Copy link
Contributor Author

#build

@LibbyBot
Copy link

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_x2opYPG4Eei52rYPrqOLXw

Target locations of links might be accessible only to IBM employees.

@kberkos-public
Copy link
Contributor Author

#build

@LibbyBot
Copy link

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_2dN2EPKAEei52rYPrqOLXw

Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

The build kberkos-public-5077-20181127-2120
https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_2dN2EPKAEei52rYPrqOLXw
completed and has errors or failures.

@kberkos-public
Copy link
Contributor Author

#build

@LibbyBot
Copy link

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_nJQmwPMjEei52rYPrqOLXw

Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

The build kberkos-public-5077-20181128-1648
https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_nJQmwPMjEei52rYPrqOLXw
completed and has errors or failures.

@kberkos-public
Copy link
Contributor Author

#build

@LibbyBot
Copy link

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_7hukgPS_Eei52rYPrqOLXw

Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

LibbyBot commented Dec 1, 2018

The build kberkos-public-5077-20181130-1826
https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_7hukgPS_Eei52rYPrqOLXw
completed and has errors or failures.

Copy link
Contributor

@kaczyns kaczyns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last build contained only previously-reported defects, or unrelated timing problems. I am approving the PR.

@kaczyns kaczyns merged commit 0bc91f9 into OpenLiberty:integration Dec 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release bug This bug is present in a released version of Open Liberty release:19001
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants