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

Fix: 404 on Hydra Oauth page #4835

Conversation

4 participants
@dancastellon
Copy link
Contributor

dancastellon commented Nov 27, 2018

Resolves #4794
Impact: major
Type: bugfix

Issue Description

OAuth IDP login pages created by the Hydra plugin are not showing up in the latest release candidate 6. So /account/login returns Not Found. Going back a few commits (e.g d37e12b2ea92), the page shows up fine.

Solution

Calling Reaction.addRolesToGroups() to add the "account/login" permission to guest & customer groups was the first step. This was called in an afterCoreInit event callback in the hydra-oauth core plugin (see @impactmass's original PR - https://github.com/reactioncommerce/reaction/pull/4795/files)

Even with that, it was taking 2 server restarts before the account/login view was working (not returning a 404). I traced this down to Migration 5 - which was deleting and recreating all groups. Because we now have a core createGroups function that does this before the migrations run, this migration is no longer needed.

Breaking changes

None

Testing

  1. Run reaction reset -n
  2. Startup Reaction via the platform and confirm you can login and signup through clicking the buttons in the starterkit
  3. Confirm you can still login as admin in Reaction and have correct permissions (update shop info, enable payment & shipping methods, etc)
  4. Confirm you can still checkout in Starterkit & Reaction

@dancastellon dancastellon requested review from aldeed and impactmass Nov 27, 2018

@dancastellon dancastellon changed the title Fix 4794 dancastellon dancastellon login route not found Fix: 404 on Hydra Oauth page Nov 27, 2018

@aldeed

This comment has been minimized.

Copy link
Member

aldeed commented Nov 28, 2018

Generally we never want to change a migration after it's been created. This may be a rare case when it's ok, but I'll have to give it some thought. If there's any way to just fix the migration to still do the same thing it was originally needed for, but without interfering with new groups, that would be preferable.

@impactmass

This comment has been minimized.

Copy link
Member

impactmass commented Nov 28, 2018

I share the same concern about not removing the migration. Flashing back, I see that the migration was added as a path for Reaction apps that existed before the introduction of Groups. I'll take a look now at how we can still keep that without the side effect we are currently getting

@impactmass

This comment has been minimized.

Copy link
Member

impactmass commented Nov 28, 2018

I find that: Migration 5 needs to be smarter. It currently assumes that every install it gets called on is coming from a pre-Groups version of Reaction. That is not always true (e.g in the case of this issue).

It needs to confirm that there are no correctly setup Groups, before it tries to run. @dancastellon we can sync on this.

@aldeed

This comment has been minimized.

Copy link
Member

aldeed commented Nov 28, 2018

This is partly a flaw with the current migration system. It should be possible to mark the current DB version while seeding it, so that migrations that are unnecessary don't run. However, we're replacing the migration system soon so if we can work around this flaw for now, that's the best solution.

@dancastellon

This comment has been minimized.

Copy link
Contributor Author

dancastellon commented Nov 28, 2018

@aldeed - @impactmass and I discussed what to do to keep the migration, and he just pushed a fix. Can you take another look? Thanks!

@aldeed
Copy link
Member

aldeed left a comment

Looks like a good solution. Just one suggestion

Comment fixed

@impactmass
Copy link
Member

impactmass left a comment

@aldeed I addressed your comment in the migration file.

I'm approving since I was listed as reviewer when it was created. Good by you to merge?

@spencern spencern merged commit 6dace5e into release-2.0.0-rc.7 Nov 29, 2018

3 checks passed

License Compliance All checks passed.
Details
WIP ready for review
Details
security/snyk - package.json (Reaction Commerce) No new issues
Details

@spencern spencern deleted the fix-4794-dancastellon-dancastellon-login-route-not-found branch Nov 29, 2018

@spencern

This comment has been minimized.

Copy link
Member

spencern commented Nov 29, 2018

Merging on @impactmass approval and @aldeed's review with a single suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.