-
Notifications
You must be signed in to change notification settings - Fork 134
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
Support separate auth and token endpoints for the console #3635
Conversation
✅ Hey andrewghobrial! The commit authors and yourself have already signed the CLA. |
Codecov Report
@@ Coverage Diff @@
## v2-master #3635 +/- ##
=============================================
- Coverage 51.42% 51.42% -0.01%
=============================================
Files 724 724
Lines 20563 20563
Branches 3681 3681
=============================================
- Hits 10575 10574 -1
- Misses 9988 9989 +1 |
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.
Hi Andrew,
I looked into the issue with e2e tests. The 503 was caused by the jetstream backend starting up without being fully configured, so the front end was unable to communicate with it. Presumably that when testing in your AIO docker container, your config file had the AUTHORIZATION_ENDPOINT set? That config was not pushed, and so it meant that the backend during the e2e tests, could not find it in the config. As a result, since your implementation relies on it, jetstream failed to start correctly, but was not aborted completely and thus the e2e tests failed.
I was able to run e2e tests by adding the AUTHORIZATION_ENDPOINT to the config file and setting it to the same value as UAA_ENDPOINT, (we use the single endpoint for both).
To remedy the issue, I suggest preserving the current working behaviour of the backend by making the AUTHORIZATION_ENDPOINT variable optional - if it is not specified, then jetstream would default to using a single UAA endpoint. However if a separate token endpoint is required, then it can be configured using the extra variable. This also avoids needing to add both AUTHORIZATION_ENDPOINT and UAA_ENDPOINT and setting them to the same value, which would now be required in the majority of use cases.
A minor change required is in src/jetstream/plugins/cloudfoundryhosting/main.go where you add an override. This just needs the log statement to be updated.
Feel free to let me know any questions you have. Cheers, Kate
// Override the configuration to set the authorization endpoint | ||
url, err = url.Parse(newCNSI.TokenEndpoint) | ||
if err != nil { | ||
return fmt.Errorf("Invalid authorization endpoint URL %s %s", newCNSI.AuthorizationEndpoint, err) |
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.
Needs an update to error message to reflect token endpoint
yeah makes sense thanks @kreinecke i will add that change to make it backwards compatible |
ok @kreinecke I made the changes. e2e tests look much better, but still a few failures that seem unrelated to this PR |
You're right, I don't think the failures are related to your PR. However Richard Cox made a commit to our v2-master just the other day, to fix some e2e test issues. I think your branch is missing just that one latest commit - could you pull it in to your branch please? The e2e test should hopefully then pass. |
Just rebased with the latest v2-master, but looks like some failures in e2e still |
OK. Thanks Andrew. In that case I will talk with the team on Monday about the current status of e2e tests, in case there is another issue. |
Hi Andrew, it was indeed a glitch with the build - tests all passing now. I'll do a final check over of your changes, then should be good to go. |
Great thanks |
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.
LGTM
@kreinecke This looks good to merge |
* entity-catalogue-store: (57 commits) Fix jasmine exception in build Fixes following merge cf: moved entity factory schemas to its module (#3657) Address todo - use common getCFEntityKey For example from EntityCatalogueHelpers.buildEntityKey(organizationSchemaKey, CF_ENDPOINT_TYPE) to getCFEntityKey(organizationSchemaKey) Add general todo's following review with Nathan Indentation fixes (will make reviewing later much easier) also includes import fixes Fix edit org/space (to allow testing of merge) Fix navigate from org summary to org spaces Fixes following merge Remove unused users.effetct.ts (#3660) Invite user e2e tests (#3382) Support separate auth and token endpoints for the console (#3635) Add e2e uaa helpers, use to create/delete user for remove roles tests (#3651) Allow appVersion in helm chart to be set to a product version (#3637) Get default stack from the Stacks API (#3648) Helm Chart: Add imagelist to the helm chart (#3638) E2E Tests: Update cleanup script to remove test users (#3644) Remove ZenHub (#3652) Disable top level remove user tests to unblock pr's (#3646) Show header in none dashboard pages ...
- Ensure we use the AuthorizationEndpoint when fetching initial token - This was recently changed in #3635 - The PR covered what we expect to be a fix to SSO/cf push for CFs where token and auth endpoints differ - It changed FetchOAuth2Token which is not used in cf push world - Change broke cf push connect to IBM - Fix is to revert the change made at only this place - More info can be found at.. - https://docs.cloudfoundry.org/api/uaa/version/4.31.0/index.html#password-grant - https://tools.ietf.org/html/rfc6749#section-3.2
- Ensure we use the AuthorizationEndpoint when fetching initial token - This was recently changed in #3635 - The PR covered what we expect to be a fix to SSO/cf push for CFs where token and auth endpoints differ - It changed FetchOAuth2Token which is not used in cf push world - Change broke cf push connect to IBM - Fix is to revert the change made at only this place - More info can be found at.. - https://docs.cloudfoundry.org/api/uaa/version/4.31.0/index.html#password-grant - https://tools.ietf.org/html/rfc6749#section-3.2
This is PR creates a new console config for Auth Endpoint. Before this PR, there was only "UAA Endpoint", which was the auth_endpoint from /v2/info. Since /v2/info contains both auth_endpint and token_endpoint and they are used for different types of requests, they should also be separate in Stratos.
Description
-Added new AuthorizationEndpoint in ConsoleConfig
-Use the AuthorizationEndpoint only for /oauth/authorize redirect
-Use the UAAEndpoint for all /oauth/token calls
-Make necessary DB changes and DB migration
Motivation and Context
-While today the authorization endpoint is used for both /oauth/authorize and /oauth/token, and this works for most of the time, we have a use case in which the authorization endpoint cannot be used to do /oauth/token. By the /v2/info standard, token_endpoint should be used to retrieve tokens, and auth_endpoint should be used for oauth redirect.
How Has This Been Tested?
I tested this using a CF and Docker AIO install on our environments.
Types of changes
Checklist: