JIT Changes #4
JIT Changes #4
Conversation
| @@ -168,7 +171,7 @@ protected Subject buildSubject(String nameIdentifierValue) | |||
| protected abstract Subject buildSubject(); | |||
|
|
|||
| protected AttributeStatement buildAttributeStatement( | |||
| List<Attribute> additionalAttributes) throws IllegalStateException { | |||
| List<Attribute> addAttributes) throws IllegalStateException { | |||
There was a problem hiding this comment.
This is confusing having both an additionalAttributes and addAttributes. In addition, addAtrributes sounds like a verb, not a noun. Beyond just renaming the var, this probably should be rafactored to have the injection of both kinds of additional attributes be cleaner. I know the original code was a bit messy though :(
There was a problem hiding this comment.
Also, what happens if someone used the form for attributes, but also set the same thing in free form JIT field? Does it matter if they are using subject or attribute for user id? See comment below about testing.
There was a problem hiding this comment.
Agree that the names are confusing, just tried to get it working with minimal changes. For now we can just rename the var and leave the refactoring as a TODO item?
There was a problem hiding this comment.
If the user selects subject there is no issues at all. Say the user selects Attribute and the name to be say "UserName", if the user now sets something like UserName=blahblahblah; in the free from JIT field, as long this value is correct the assertion would work.
There was a problem hiding this comment.
Just a thought, if the user selects "Attribute" radio button, what if we disable Username or Federated ID text field and basically force the user to add the attribute in the Additional Attributes text area. If we go that route, we can remove the Attribute Name and Format fields from the UI. In essence we would have all attributes defined in the additional attributes area and have one method to construct the attributes.
|
Are there tests for this change? |
|
Nope, need to add them... -----Original Message----- Are there tests for this change? Reply to this email directly or view it on GitHub: |
…for attributes other thatn additional Attributes
Ryan,
Primarily added text area to input additional attributes on the Response Request page and another text area to format the SAML response to make it more readable on the Target Poster page. Please let me know if you have any questions.
Thanks,
Mahanthi