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

feat: Add a CORS-enabled endpoint for token refresh in Hydra plugin #4743

Merged
merged 2 commits into from
Oct 18, 2018

Conversation

impactmass
Copy link
Contributor

@impactmass impactmass commented Oct 16, 2018

Part of reactioncommerce/example-storefront#350
Type: feature

Issue

To help enable refresh token in Starterkit (reactioncommerce/example-storefront#350)

Changes

  • Adds CORS-enabled endpoint to be called from Starterkit for token refresh

Breaking changes

n/a

Testing

  1. Test steps in feat: Refresh access tokens on 401 errors example-storefront#394

@impactmass impactmass changed the title WIP - Feat 350 impactmass refresh token WIP - feat: Add a CORS-enabled endpoint for token refresh in Hydra plugin Oct 16, 2018
@impactmass impactmass force-pushed the feat-350-impactmass-refresh-token branch from f267127 to d01e62f Compare October 16, 2018 14:44
@impactmass impactmass changed the title WIP - feat: Add a CORS-enabled endpoint for token refresh in Hydra plugin feat: Add a CORS-enabled endpoint for token refresh in Hydra plugin Oct 16, 2018
@impactmass impactmass modified the milestones: Pikes Peak, Oxford Oct 16, 2018
@impactmass impactmass requested a review from mikemurray October 16, 2018 15:05
@impactmass impactmass self-assigned this Oct 16, 2018
@impactmass impactmass changed the title feat: Add a CORS-enabled endpoint for token refresh in Hydra plugin WIP - feat: Add a CORS-enabled endpoint for token refresh in Hydra plugin Oct 17, 2018
@impactmass impactmass removed the request for review from mikemurray October 17, 2018 11:08
@aldeed aldeed changed the title WIP - feat: Add a CORS-enabled endpoint for token refresh in Hydra plugin feat: Add a CORS-enabled endpoint for token refresh in Hydra plugin Oct 17, 2018
aldeed
aldeed previously requested changes Oct 17, 2018
@@ -68,6 +68,19 @@ WebApp.connectHandlers.use("/consent", (req, res) => {
.catch((errorMessage) => errorHandler(errorMessage, res));
});

WebApp.connectHandlers.use("/token/refresh", (req, res) => {
res.setHeader("Access-Control-Allow-Origin", process.env.OAUTH2_CLIENT_DOMAIN);
Copy link
Contributor

Choose a reason for hiding this comment

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

According to this, you need the Vary: Origin header, too: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin#CORS_and_caching

There would likely be many different domains hitting this. I believe the way this is usually done is setting the value to req.headers.origin after verifying that that origin is allowed.

Maybe change ENV name to DOMAINS plural, split it to an array on commas, and then check whether the request origin header is in that array?

if (res.status < 200 || res.status > 302) {
const json = await res.json();
Logger.error("An error occurred while calling refresh API", json.error_description);
return Promise.reject(new Error(json.error_description));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an odd mix of normal promise and async/await. It seems like you could rewrite to be completely async/await:

async function refreshAuthToken({ refreshToken, clientId, clientSecret }) {
  const res = await fetch(`${HYDRA_TOKEN_URL}`, {
    headers: { "Content-Type": "application/x-www-form-urlencoded" },
    method: "POST",
    body: `grant_type=refresh_token&refresh_token=${refreshToken}&response_type=token&client_id=${clientId}&client_secret=${clientSecret}`
  });

  const json = await res.json();

  if (res.status < 200 || res.status > 302) {
    Logger.error("An error occurred while calling refresh API", json.error_description);
    throw new Error(json.error_description);
  }

  return json;
}

export default {
getLoginRequest: (challenge) => get("login", challenge),
acceptLoginRequest: (challenge, body) => put("login", "accept", challenge, body),
rejectLoginRequest: (challenge) => put("login", "reject", challenge),
getConsentRequest: (challenge) => get("consent", challenge),
acceptConsentRequest: (challenge, body) => put("consent", "accept", challenge, body),
rejectConsentRequest: (challenge, body) => put("consent", "reject", challenge, body),
deleteUserSession: (id) => deleteUserSession(id)
deleteUserSession: (id) => deleteUserSession(id),
refreshAuthToken: (options) => refreshAuthToken(options)
Copy link
Contributor

Choose a reason for hiding this comment

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

For these last two, they are not adding any arguments and are named the same, so you can eliminate a function layer by instead doing:

// ...
  deleteUserSession,
  refreshAuthToken
}

@spencern spencern removed this from the Oxford milestone Oct 18, 2018
@impactmass impactmass dismissed aldeed’s stale review October 18, 2018 16:48

@aldeed thanks for the great feedback. I've addressed the comments here, but still yet to sort out the cookie issue on the other related PR. Decisions there may still require some changes here

@aldeed aldeed changed the base branch from release-2.0.0-rc.5 to release-2.0.0-rc.6 October 18, 2018 21:35
@aldeed aldeed merged commit 7758ea2 into release-2.0.0-rc.6 Oct 18, 2018
@aldeed aldeed deleted the feat-350-impactmass-refresh-token branch October 18, 2018 21:37
@aldeed
Copy link
Contributor

aldeed commented Oct 18, 2018

@impactmass I went ahead and merged this into RC6 to keep things moving. If changes are necessary based on starter kit decisions, let's do a new PR for those.

@spencern spencern mentioned this pull request Oct 20, 2018
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.

3 participants