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

PR for ES-1843 #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

PR for ES-1843 #37

wants to merge 1 commit into from

Conversation

mathiasconradt
Copy link

✏️ Changes

The "Try" button of a custom social connection ignored the scope configuration entirely, because it was hardcoded. This should fix it, but please review, I wasn't able to deploy/test this extension anywhere. This PR is based on looking at the code.

📷 Screenshots

See https://auth0team.atlassian.net/servicedesk/customer/portal/34/ESD-1843

🔗 References

See https://auth0team.atlassian.net/servicedesk/customer/portal/34/ESD-1843

🎯 Testing

See https://auth0team.atlassian.net/servicedesk/customer/portal/34/ESD-1843

Configure a custom social connection and change the default scope configuration. The scopes should not be ignored when running the "Try" command.
Before, it was hardcoded as 'openid profile' and ignoring any additional scopes set in the configuration.

✅🚫 This change has been tested in a Webtask

✅🚫 This change has unit test coverage

✅🚫 This change has integration test coverage

✅🚫 This change has been tested for performance

🚀 Deployment

Can this change be merged at any time? What will the deployment of the change look like? Does this need to be released in lockstep with something else?

✅🚫 This can be deployed any time

or
⚠️ This should not be merged until:

  • Other PR is merged because REASON
  • After date because REASON
  • Other condition: REASON

🎡 Rollout

Explain how the change will be verified once released. Manual testing? Functional testing?

In order to verify that the deployment was successful we will …

🔥 Rollback

Explain when and why we will rollback the change.

We will rollback if …

📄 Procedure

Explain how the rollback for this change will look like, how we can recover fast.

🖥 Appliance

Please review carefully, I wasn't able to deploy/test this extension anywhere. This PR is based on looking at the code.

@@ -85,7 +85,7 @@ var ConnectionModal = React.createClass({
return [
window.env.userUrl + '?',
'response_type=code',
'&scope=openid%20profile',
'&scope=' + encodeURIComponent(this.state.connection.options.scope),
Copy link
Contributor

Choose a reason for hiding this comment

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

this.state.connection does not update on _save. So you have to re-open modal dialog to update the URL.
I'd suggest this:

var connection = this.state.connectionForm && this.state.connectionForm.getConnection() || this.state.connection;
return [
      window.env.userUrl + '?',
      'response_type=code',
      '&scope=' + encodeURIComponent(connection.options.scope),
      '&client_id=' + window.env.masterClientId,
      '&connection=' + this.state.connection.name,
      '&redirect_uri=' + window.env.manageUrl + '/tester/callback?connection=' + this.state.connection.name
    ].join('');

Copy link
Author

Choose a reason for hiding this comment

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

I have no way to really test this properly, I have no local test environment. I also am not familiar with this code base of the extension. Please consider my suggestion just as pseudo-code then :)
Happy to go with your suggestion if it works, as I don't have a way to test it. I trust a product engineer here more than my own code.

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

Successfully merging this pull request may close these issues.

2 participants