Added login_hint for Password Grant Token issue 923 #924
Conversation
|
❌ Hey pavellom! All pull request submitters and commit authors must have a Contributor License Agreement (CLA). Click here for details on the CLA process. The following github user @pavellom is not covered by a CLA. After the CLA process is complete, this pull request will need to be closed & reopened. DreddBot will then validate the CLA(s). |
ghost
mentioned this pull request
ghost
closed this
ghost
reopened this
|
✅ Hey pavellom! The commit authors and yourself have already signed the CLA. |
|
Hi @twoseat, Can you please review this change? |
twoseat
left a comment
twoseat
left a comment
There was a problem hiding this comment.
Given the nature of the change it makes sense to me to apply it to all the requests that have added login_hint, not just for Password Grant.
| abstract String getUsername(); | ||
|
|
||
| /** | ||
| * The login_hint |
There was a problem hiding this comment.
Javadoc should use English, e.g. "The identity provider to be used", or at least "The login hint"
| /** | ||
| * The login_hint | ||
| */ | ||
| abstract String getLoginHint(); |
There was a problem hiding this comment.
Field should be marked Nullable, as it's optional
| /** | ||
| * The login_hint | ||
| */ | ||
| abstract String getLoginHint(); |
There was a problem hiding this comment.
I'm unsure about this, but I don't think we should treat this as a simple string. If we do that it relies on the user to provide a correctly encoded json object, when they could provide an endpoint (still as a string) that we convert to a correctly encoded json object for them. So the difference between them providing
%257B%2522origin%2522%253A%2522uaa%2522%257D
rather than
uaa
I'd be interested in your thoughts, and those of @andreaAlkalay as users.
There was a problem hiding this comment.
To highlight the potential confusion that first example might actually need to be %7B%22origin%22%3A%22uaa%22%7D, I'm still wrapping my head around that.
There was a problem hiding this comment.
Hi,
This was exactly my thought at first, but then I thought, what would happen if another parameter will be needed in the wrapped json? What if the existing parameter wiil be renamed from origin to (I don't know) TheOrigin?
I think if UAA asked to provide json as value to parameter called login_hint and not added parameter called origin, is an indication that this is not final,
and login_hint value could be extended for other purposes other than origin.
If this is the case, I'm not sure that constructing json by ourself rather then asking to provide it as is, is something we would like to handle.
There was a problem hiding this comment.
Hi,
I agree with Pavel, the solution he suggests is aligned with the UAA implementation and provides an accurate implementation in the CF client.
Thanks a lot,
Andrea.
There was a problem hiding this comment.
@twoseat Regarding the encoding you are right, it should be %7B%22origin%22%3A%22uaa%22%7D. For some reason the UAA API documentation seems to be wrong (double-encoded) here. @tack-sap and myself (we developed the feature in the UAA) will check and correct it on the UAA API doc side.
The idea was indeed to pass a JSON as login_hint so that other login hints (apart from origin) could still be added later on.
There was a problem hiding this comment.
Thanks @torsten-sap that's reassuring to know - it seems the example shows an encoded json url inside an encoded url, hence my uncertainty about where the double-encode was coming from. Either way the doc would benefit from a clearer example!
There was a problem hiding this comment.
Hi @twoseat,
Are you ok with this change?
If so, when do you expect it to be released?
There was a problem hiding this comment.
The change looks good, I just need to expand it to include the other occurrences of login_hint. With luck I should be able to include that tomorrow, so you should see a snapshot build soon.
|
This was merged in 68ec16b, I'm not sure why the PR wasn't automatically closed. |
4 participants
No description provided.