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

Support separate auth and token endpoints for the console #3635

Merged
merged 4 commits into from
Jun 17, 2019

Conversation

andrewghobrial
Copy link
Contributor

@andrewghobrial andrewghobrial commented Jun 5, 2019

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

  • Bug fix (non-breaking change which fixes an issue)
  • Docs update
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • [ X] My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have followed the guidelines in CONTRIBUTING.md, including the required formatting of the commit message

@cfdreddbot
Copy link

✅ Hey andrewghobrial! The commit authors and yourself have already signed the CLA.

@codecov
Copy link

codecov bot commented Jun 5, 2019

Codecov Report

Merging #3635 into v2-master will decrease coverage by <.01%.
The diff coverage is n/a.

@@              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

@kreinecke kreinecke self-requested a review June 7, 2019 07:51
Copy link
Contributor

@kreinecke kreinecke left a 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)
Copy link
Contributor

@kreinecke kreinecke Jun 7, 2019

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

@andrewghobrial
Copy link
Contributor Author

yeah makes sense thanks @kreinecke i will add that change to make it backwards compatible

@andrewghobrial
Copy link
Contributor Author

ok @kreinecke I made the changes. e2e tests look much better, but still a few failures that seem unrelated to this PR

@kreinecke
Copy link
Contributor

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.

@andrewghobrial
Copy link
Contributor Author

Just rebased with the latest v2-master, but looks like some failures in e2e still

@kreinecke
Copy link
Contributor

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.

@kreinecke
Copy link
Contributor

kreinecke commented Jun 11, 2019

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.

@andrewghobrial
Copy link
Contributor Author

Great thanks

Copy link
Contributor

@nwmac nwmac left a comment

Choose a reason for hiding this comment

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

LGTM

@nwmac
Copy link
Contributor

nwmac commented Jun 12, 2019

@kreinecke This looks good to merge

@kreinecke kreinecke merged commit 1d1dd07 into cloudfoundry:v2-master Jun 17, 2019
KlapTrap added a commit that referenced this pull request Jun 19, 2019
* 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
  ...
richard-cox added a commit that referenced this pull request Jul 16, 2019
- 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
nwmac pushed a commit that referenced this pull request Jul 17, 2019
- 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants