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 flag to enable only IDP routes #4903

Merged
merged 6 commits into from
Jan 14, 2019

Conversation

impactmass
Copy link
Contributor

@impactmass impactmass commented Jan 8, 2019

Resolves #4852
Impact:
Type: feature

Issue

See #4852

Goals for this PR:

  • Ability to run the app in a mode that allows only routes from the Identity Provider Plugin

Solution Overview

  • New ENV var IDENTITY_PROVIDER_MODE that is used to filter published routes

Details

  • There are three values the ENV can be set to. Here's what they do:
    • Showing all routes (no restriction) (ENV value all)
    • Showing only Identity provider routes (ENV value idp-only)
    • Showing all routes excluding identity provider routes (ENV value exclude-idp)
  • In the client router, the code logic that creates the routes array from package registry items is now wrapped as a function, so that it can be called with different args.

Breaking changes

None

Testing

Note: Make sure to remove existing reaction container after changing env value (e.g with docker-compose down and then up again).

  1. Set IDENTITY_PROVIDER_MODE=all in your ENV.
    Once app is started, we want to confirm that all routes are available. Home page should load the products and /account/login should show login form.

  2. Set IDENTITY_PROVIDER_MODE=idp-only in your ENV.
    Once app is started, we want to confirm that only IDP routes show. Viewing /account/login should show login form. Other usual pages (including home page) show Not Found.

  3. Set IDENTITY_PROVIDER_MODE=exclude-idp in your ENV.
    Once app is started, we want to confirm that IDP routes do not show. Viewing /account/login should not show up. Other usual pages (including home page) should show.

@impactmass impactmass changed the title WIP - feat: Add flag to enable only WIP - feat: Add flag to enable only IDP routes Jan 9, 2019
@impactmass impactmass force-pushed the feat-4852-impactmass-router-and-idp branch from 1878195 to e4762ac Compare January 9, 2019 17:25
},
description: "Not Found Page",
workflow: "hydraOauthLogin",
template: "notFound"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IDP's Not Found route definition. Here it's using the existing/default template. It can be updated to use a custom template as needed

@@ -7,6 +7,6 @@ Hooks.Events.add("afterCoreInit", () => {
Reaction.addRolesToGroups({
allShops: true,
groups: ["guest", "customer"],
roles: ["account/login"]
roles: ["account/login", "not-found"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the IDP defines the Not Found registry route, it needs to grant the access to it.

@impactmass impactmass changed the title WIP - feat: Add flag to enable only IDP routes feat: Add flag to enable only IDP routes Jan 9, 2019
@impactmass impactmass requested a review from mikemurray January 9, 2019 19:16
const query = { shopId: myShopId };

// This is to ensure only needed Identity-provider-related routes are published
// The env can be one of three: "all", "idp-only", "exclude-idp". Default behavior is "all"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ticean I added you for comments about the ENV values. Is the naming good, unambiguous etc?

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me!

@impactmass impactmass requested a review from ticean January 10, 2019 12:00
Copy link
Member

@mikemurray mikemurray left a comment

Choose a reason for hiding this comment

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

👍

Failing test is being addressed in this ticket #4909

@mikemurray
Copy link
Member

@impactmass approved on my end. You're free to merge whenever you get the 👍 from @ticean.

const query = { shopId: myShopId };

// This is to ensure only needed Identity-provider-related routes are published
// The env can be one of three: "all", "idp-only", "exclude-idp". Default behavior is "all"
Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me!

@impactmass impactmass merged commit c5e8d50 into develop Jan 14, 2019
@impactmass impactmass deleted the feat-4852-impactmass-router-and-idp branch January 14, 2019 12:42
@spencern spencern mentioned this pull request Jan 18, 2019
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