-
Notifications
You must be signed in to change notification settings - Fork 827
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
test: Authorization Grant Flow without Redirect URI #2484
Conversation
Add test for this hidden feature thus not to run into a regression
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/186021972 The labels on this github issue will be updated when the story is started. |
@@ -82,7 +82,7 @@ | |||
<entry key="secret" value="loginsecret"/> | |||
<entry key="scope" value="openid,oauth.approvals"/> | |||
<entry key="authorized-grant-types" value="client_credentials,authorization_code"/> | |||
<entry key="redirect-uri" value="http://localhost/**,http://localhost:7000/**"/> |
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.
What is the purpose of this change? Is this required for the added test?
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 mainly is the revert of my change https://github.com/cloudfoundry/uaa/pull/2403/files#diff-d02078cfeded6b34f4eb0a68b222758f944db9bbe2fa1227249a9c782718cf09R85
the login client is used in our internal forked scenario and it was broken. Because UAA has this hidden feature of redirect uris. It means if you have only one redirect uri defined you can omit the parameter redirect_uri in authorize request.
If you have an array in the client you cannot use this feature anymore therefore removed the 2nd entry
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.
and create an explicit test for this hidden feature
Map<String, String> body = IntegrationTestUtils.getAuthorizationCodeTokenMap(serverRunning, | ||
testAccounts, | ||
"login", | ||
"loginsecret", |
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 client has only one redirect uri therefore you can omit it.
|
||
try { | ||
IntegrationTestUtils.getAuthorizationCodeTokenMap(serverRunning, testAccounts, "app", "appclientsecret", | ||
testAccounts.getUserName(), testAccounts.getPassword(), |
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.
the app client has 2 entries for redirect-uri thus here it does not work.
These tests simply should help to know what we have already, nothing new
Add test for this hidden feature thus not to run into a regression