Use secure token in path instead of reg. identifier #579
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cintamani
force-pushed
the
secure-token
branch
2 times, most recently
from
548d487
to
10f65ed
Compare
Cruikshanks
force-pushed
the
secure-token
branch
5 times, most recently
from
f65b6d2
to
108628b
Compare
Use the new service we added which will generate a 24 char just like WEX.
Finally figured out how to get something passing 😅
Main fix was ensuring the path was correct in the views. Whilst working on this also spotted that some of our tests were not quite testing the right thing. Previouslyh we were passing in invalid params (first and last name are blank) along with an invalid reg identifier. With the help of @irisfaraway detective skills we could see that the reg identifier was always short circuting any validation checks and causing a redirect to 'Registration is invalid' page, hence the 302 code. If you remove invalid reg identifiers from the equation (and in the new world tokens) what actually happens when invalid params like a blank first name and last name are submitted is that we return a 200 and simply `render :new`. So this change also removes any tests that were just checking for a 302 code. N.B. It's likely we have other tests that make the same assumption being removed as I work through this.
Similiar to conviction details, some of the invalid params 302 tests have been removed as they are no longer applicable or correct.
TBH I compared this to the other location form specs and found it had a bunch of tests the others didn't. I just deleted them and woo-hoo, all tests pass!
Issue was actually an error in the controller which this refactoring seems to have exposed.
This required a tweak elsehwere than just the specs. In the renewal completion service. Just needed to ensure we weren't trying to copy our new token field to the Registration instance. The rest was spec changes. We'd been a bit to vigorous swapping `reg_identifier` for `token` so this fixed those. The key change though is in tweaking the tests to match what now happens if a user is on the completion page and they hit refresh. Previously the code would have directed them to the renewal start page, where they then immediately got redirected to the unrenewable page. Now though we're not passing a reg identifier, we're passing a token. This means the base form controlller validation tries to find the renewing transient registration, can't so ends up redirecting us to the invalid page. There's no obvious way we can get round this if we are going with using the token (plus I have a suspicion this is what we do in WEX as well for the same reasons). So this change re-jigs the tests to try match the behaviour, and to try and reflect what is actually going on.
Change is a result of dealing with merge conflicts after updating branch to the latest master.
The changes here were mainly two-fold. The first was updating the controller to overrider the base form controller's `find_or_initialize_transient_registration()` with one that would consider the token as an actual token, or a registration identifier. The second was then going back through the spec and ensuring we were referring to a token or a registration identifier, rather than using the token for everything.
Noting here as I think it will effect a number of forms. It looks like we were testing invalid params by sending an invalid registration ID before. Now we are using the token, and it will form part of the url and not be a param, what we really want to test is something the underlying form is actually expected to submit.
Key change is the WorldpayUrlService needs to be dealing with a transient registration token not a reg identifier, so we can accurately determine which one to assign a payment against.
Cruikshanks
force-pushed
the
secure-token
branch
from
6312500
to
6a1d5b3
Compare
Spacing and line length issues only
There is an argument that rubocop is right and we _should_ rename this method. But if so think its a refactor for another time. For now this is adding an exception.
Based on how things were tracked previously, we had the reg identifier as a hidden field in each form. Now we rely on the token being the first part of the url so shouldn't need this hidden field. In order to make the views cleaner this change removes those hidden fields.
We were passing the reg identifier as an additional param when deleting a person. However we don't believe the form or controller actually need this information so removing it from the view.
Spotted that we seemed to be missing a tag in one of the views I touched.
Because it was right and was not missing you plonker @Cruikshanks
Cruikshanks
changed the title
Cruikshanks
added a commit
to DEFRA/waste-carriers-frontend
that referenced
this pull request
Dec 11, 2019
We had to implement a change to how urls, and specifically any url that points to a page which handles transient registrations are formed. DEFRA/waste-carriers-engine#579 Essentially we needed to switch to using a token rather than the registration identifier. That specific change does not affect the frontend, but as part of the same PR we also fixed an issue with validation errors, where generating one would break back links. This turned out to just be because we were putting the ID for a 'resource' at the end of the url, rather than before the action we wanted to carry out against it. This seems to break a rails convention hence generated back links stopped working in certain cases. That fix does impact this project, because it now needs to ensure that the path it redirects users to when renewing matches what the new front and back office apps expect. This change covers everything needed to update the frontend to redirect users correctly.
Cruikshanks
added a commit
to DEFRA/waste-carriers-frontend
that referenced
this pull request
Dec 12, 2019
We had to implement a change to how URLs, and specifically any URL that points to a page which handles transient registrations are formed. DEFRA/waste-carriers-engine#579 Essentially we needed to switch to using a token rather than the registration identifier. That specific change does not affect the frontend, but as part of the same PR, we also fixed an issue with validation errors, where generating one would break backlinks. This turned out to just be because we were putting the ID for a 'resource' at the end of the URL, rather than before the action we wanted to carry out against it. This seems to break a rails convention hence generated back links stopped working in certain cases. That fix does impact this project because it now needs to ensure that the path it redirects users to when renewing matches what the new front and back-office apps expect. This change covers everything needed to update the frontend to redirect users correctly. N.B. This also removes the check whether to show the renewals holding page PR #278 removed the renewals holding page. We should have probably removed the check whether to show the holding page as well, but forgot we had one!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We spotted when working on implementing the ability to order copy cards in the back-office that we would face an issue trying to reuse forms where the expectation was to find a transient registration using just the registration identifier.
For example, CBDU100 is an active registration. I then start a renewal so create a
RenewingRegistrationwhich sits in thetransient_registrationscollection. But then I need to take a copy card order for the existing active registration. That would create anOrderCopyCardsRegistrationtransient registration with the same registration identifier of CBDU100.So how would the renewing journey or the order copy cards journey know which transient registration to pull if we only have the reg_identifier as an ID?
We solved this in WEX by having all transient registrations automatically creating a
token(which is a Base58 24 character string) when first saved to the DB, and then using that in the URL to identify the record.So this change, which touches loads of files in the codebase, actually is about switching to using the token rather than the registration identifier in our journeys.
It also brings the URL pattern inline with WEX, so
/{token}/stage/. This resolves an issue we had in WEX and WCR with validations losing the ID (because it was at the end of the URL rather than the beginning) which then broke going back after a validation error.