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

Allowing multiple redirectSignIn/redirectSignOut urls breaks federated auth #2792

Open
djheru opened this issue Aug 21, 2019 · 10 comments
Open

Allowing multiple redirectSignIn/redirectSignOut urls breaks federated auth #2792

djheru opened this issue Aug 21, 2019 · 10 comments

Comments

@djheru
Copy link

@djheru djheru commented Aug 21, 2019

Describe the bug
Using the Amplify CLI to add multiple redirectSignIn/redirectSignOut URLs results in a broken config

To Reproduce
Steps to reproduce the behavior:

  1. Use amplify add auth to set up a federated auth config
  2. Add a redirectSignIn url, e.g. http://localhost:3000
  3. Add an additional redirectSignIn url, e.g. https://d32tfey1ge36f1.cloudfront.net/
  4. Add the same for redirectSignOut urls
  5. See the config has the two urls joined by a comma, e.g. "redirectSignIn": "http://localhost:3000/,https://d32tfey1ge36f1.cloudfront.net/",
  6. Attempt to authenticate in the app and see the error due to the comma-separated urls not being a valid format for the OAuth provider

Expected behavior
The CLI should not allow multiple URLs to be entered if that results in a non-working config

Additional context
Using Google as the OAuth provider, the following invalid URI is generated due to the multiple comma-separated urls being set as the redirect_uri value:

https://example-dev.auth.us-west-2.amazoncognito.com/oauth2/authorize?redirect_uri=http%3A%2F%2Flocalhost%3A3000%2F%2Chttps%3A%2F%2Fd32tfey1ge36f1.cloudfront.net%2F&response_type=code&client_id=23r4j2asdfelvhasdfas8md4t&identity_provider=Google&scopes=phone%2Cemail%2Copenid%2Cprofile%2Caws.cognito.signin.user.admin&state=ymIeVUG2ez8pbasdfasdfWiG1u42Frn&code_challenge=asdfasdfasdfb7aLeB8FAQv_V7P9TTyZzGasdfasffj0Us&code_challenge_method=S256

Notice the redirect_uri parameter has both URLs.

I solved this problem in my react.js app by overriding the awsmobile config something like this:

import awsmobile from './aws-exports';

const { NODE_ENV } = process.env;
const DEFAULT_URL = 'http://localhost:3000/';

if (NODE_ENV === 'development') {
  awsmobile.oauth.redirectSignIn = DEFAULT_URL;
  awsmobile.oauth.redirectSignOut = DEFAULT_URL;
}
export default awsmobile;
@sammartinez sammartinez transferred this issue from aws-amplify/amplify-js Aug 22, 2019
@kaustavghosh06

This comment has been minimized.

Copy link
Contributor

@kaustavghosh06 kaustavghosh06 commented Aug 22, 2019

Multiple signin and signout URI's need to be supported from the JS Lib as it's supported by the cognito service. Transferring over the JS Lib team.

@kaustavghosh06 kaustavghosh06 transferred this issue from aws-amplify/amplify-cli Aug 22, 2019
@sammartinez

This comment has been minimized.

Copy link

@sammartinez sammartinez commented Aug 23, 2019

Marking this as a feature request as it is something that is not implemented today.

@arelaxtest

This comment has been minimized.

Copy link

@arelaxtest arelaxtest commented Aug 25, 2019

Sorry guys but in fact, this is a normal behavior.

Setup amplify add auth with two URLs: "http://localhost:3000/,https://master.xxx.amplifyapp.com/" to add the sign in/sign out URLs, amplify push then git commit & git push to make the amplify console pick up the changes.

Then you get a redirect_mismatch error locally and online https://master.xxx.amplifyapp.com/

Why ?
There is no way for the react app. to know by default which URLs to use when you have two or more URLs. You must inform the app. to use one of these URLs. You can do it like that:

What can you do ?
After looking in the doc., you find pretty much a solution here: https://aws-amplify.github.io/docs/js/authentication#react-components

import Amplify, { Auth } from 'aws-amplify'
import config from './aws-exports'

and,

// copied from serviceWorker.js to know if it is localhost or not
const isLocalhost = Boolean(
  window.location.hostname === 'localhost' ||
    // [::1] is the IPv6 localhost address.
    window.location.hostname === '[::1]' ||
    // 127.0.0.1/8 is considered localhost for IPv4.
    window.location.hostname.match(
      /^127(?:\.(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)){3}$/
    )
);

// by default, say it's localhost
const oauth = {
  domain: 'xxx.auth.us-east-2.amazoncognito.com',
  scope: ['phone', 'email', 'profile', 'openid', 'aws.cognito.signin.user.admin'],
  redirectSignIn: 'http://localhost:3000/',
  redirectSignOut: 'http://localhost:3000/',
  responseType: 'code' // or 'token', note that REFRESH token will only be generated when the responseType is code
};

// if not, update the URLs
if (!isLocalhost) {
  oauth.redirectSignIn = 'https://master.xxx.amplifyapp.com/';
  oauth.redirectSignOut = 'https://master.xxx.amplifyapp.com/';
}

// copy the constant config (aws-exports.js) because config is read only.
var configUpdate = config;
// update the configUpdate constant with the good URLs
configUpdate.oauth = oauth;
// Configure Amplify with configUpdate
Amplify.configure(configUpdate);

Full code & example
You can find a demo here that also work in localhost: https://master.d3h5j4begww46c.amplifyapp.com/
And a github fork (from dabit3): https://github.com/arelaxtest/amplify-auth-demo

Next steps
More generally, you can do something like:

var urlsIn = config.oauth.redirectSignIn.split(",");
var urlsOut = config.oauth.redirectSignOut.split(",");
const oauth = {
  domain: config.oauth.domain,
  scope: config.oauth.scope,
  redirectSignIn: config.oauth.redirectSignIn,
  redirectSignOut: config.oauth.redirectSignOut,
  responseType: config.oauth.responseType
};
var hasLocalhost  = (hostname) => Boolean(hostname.match(/localhost/) || hostname.match(/127(?:\.(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)){3}/));
var hasHostname   = (hostname) => Boolean(hostname.includes(window.location.hostname));
var isLocalhost   = hasLocalhost(window.location.hostname);
if (isLocalhost) {
  urlsIn.forEach((e) =>   { if (hasLocalhost(e)) { oauth.redirectSignIn = e; }});
  urlsOut.forEach((e) =>  { if (hasLocalhost(e)) { oauth.redirectSignOut = e; }});
}
else {
  urlsIn.forEach((e) =>   { if (hasHostname(e)) { oauth.redirectSignIn = e; }});
  urlsOut.forEach((e) =>  { if (hasHostname(e)) { oauth.redirectSignOut = e; }});
}
var configUpdate = config;
configUpdate.oauth = oauth;
Amplify.configure(configUpdate);
@djheru

This comment has been minimized.

Copy link
Author

@djheru djheru commented Sep 27, 2019

I'd like to first comment that I'm really impressed with this library, and I'd like to give a hearty thank you to the teams that have added so many useful, well-integrated, and multi-faceted features

I disagree that this is "normal behavior". The CLI has the capability to add multiple redirectSignIn/redirectSignOut values to the config, but it adds them in such a way that breaks the functionality of the app.

The code you posted does serve as a workaround to this issue, but it is not really satisfactory, because you're hard-coding values in the code that overwrite the generated values in the config. This defeats the purpose of using the CLI and the config generation capabilities.

In my opinion, supporting the multiple redirect urls, and detecting when to use which is an implementation detail that should be abstracted away by the library.

@sammartinez sammartinez transferred this issue from aws-amplify/amplify-js Nov 20, 2019
@ammarkarachi ammarkarachi added the auth label Nov 20, 2019
@anticlockwise

This comment has been minimized.

Copy link

@anticlockwise anticlockwise commented Dec 16, 2019

+1 to @djheru . In addition, Amplify allows multiple environments, for each of which there is a separate cognito user pool created. The whole point of aws-exports.js generated during build time was for it to figure out what parameters to configure Amplify with. If we cannot figure out what redirectURI to use per environment during build time, that kinda defeats the purpose of this aws-exports.js being generated.

@ajhool

This comment has been minimized.

Copy link

@ajhool ajhool commented Dec 31, 2019

I think that the most dynamic solution would be to have named/aliased redirects that are stored in amplify config and added to the aws-exports file. This would allow for normalization across multiple environments and also enable selection of which redirect path to use.

For me, my main challenge is always to use localhost vs the actual hosted webapp. That would look something like:

// config for dev amplify environment
redirectUris: {
  localhost: localhost:8080,
  localhostDashboard: localhost:8080/dashboard,
  awsHosted: dev.example.org,
  awsHostedDashboard: dev.example.org/dashboard
}
// config for prod amplify environment
redirectUris: {
  localhost: localhost:8080,
  localhostDashboard: localhost:8080/dashboard,
  awsHosted: www.example.org,
  awsHostedDashboard: dev.example.org/dashboard
}
// Amplify javascript call
Auth.federatedSignIn({ redirect: 'awsHosted' })

This would allow us to whitelist all of those various domains in the Cognito Hosted UI and social providers but also select them conveniently in the frontend code without duplicate sources of truth.

I also noticed while typing this that the aws-exports file does not include an environment name (eg. env: 'dev'). This seems like a missing feature that would simplify this context management n the frontend when Amplify doesn't do so automatically.

@bitshop

This comment has been minimized.

Copy link

@bitshop bitshop commented Jan 3, 2020

It sounds more like amplify publish should update it's aws-exports.js before uploading to point to the URL based on aws_content_delivery_url in same file - and that npm start should use the aws-exports.js localhost always (i.e. not updating the file).

That seems much simpler than our code dealing with it or hard coding - Am I missing something? (New to Amplify).

@kernwig

This comment has been minimized.

Copy link

@kernwig kernwig commented Jan 12, 2020

Here's my workaround to this problem, should it be useful to someone else. It applies to Angular, but I'm sure can be adapted to any other front-end toolset:

I added this hack to the Angular main.ts file:

import awsconfig from './aws-exports';

// Choose an OAuth config based on environment
const redirectSignInOptions = awsconfig.oauth.redirectSignIn.split(',');
const redirect = environment.production
   ? redirectSignInOptions.find(s => s.startsWith('https'))
   : redirectSignInOptions.find(s => s.includes('localhost'));
awsconfig.oauth.redirectSignIn = redirect;
awsconfig.oauth.redirectSignOut = redirect;

This selects the locahost URL for development, and the https CloudFront URL for production. My sign-in and sign-out URLs are identical; if yours are different you'll need to process them independently.

@djheru

This comment has been minimized.

Copy link
Author

@djheru djheru commented Jan 17, 2020

I ended up with a similar solution, but it works just fine

import awsmobile from './aws-exports';

const { host } = window.location;

// Fix issues with multiple redirect urls.
// Try to figure out which one to use...
if (awsmobile.oauth.redirectSignIn.includes(',')) {
  const filterHost = url => new URL(url).host === host;
  awsmobile.oauth.redirectSignIn = awsmobile.oauth.redirectSignIn
    .split(',')
    .filter(filterHost)
    .shift();
  awsmobile.oauth.redirectSignOut = awsmobile.oauth.redirectSignOut
    .split(',')
    .filter(filterHost)
    .shift();
}
export default awsmobile;
@dbhagen

This comment has been minimized.

Copy link

@dbhagen dbhagen commented Feb 18, 2020

@djheru you pretty much read my mind on this script. Thanks for developing it and posting it so I didn't have to!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants
You can’t perform that action at this time.