Clarify in email hint text that shared inbox is not allowed #1746
Conversation
- Changed the password field's validation message to clarify what a hard to guess password is - Fixed the aria-invalid attribute on text boxes - Add cypress tests for registration page - Use jsrsasign to generate JWTs for admin api access in cypress tests
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
🧪 Review environmenthttps://u7jw7piycnapglfaav2es2dzky0aawyd.lambda-url.ca-central-1.on.aws/ |
| @@ -9,12 +9,12 @@ | |||
| "cypress-recurse": "^1.23.0", | |||
| "html-validate": "^8.7.1", | |||
| "imap-simple": "^5.1.0", | |||
| "jsonwebtoken": "^9.0.0", | |||
| "jsrsasign": "^11.0.0", | |||
There was a problem hiding this comment.
This was tricky to get working. jsonwebtoken depends on the crypto node package which is server side, not browser facing. Suggestions online were to use polyfills to inject cyrpto into Cypress via cypress.config.js which in turn required additional node builtins like os and process. Switching to a pure JS lib made more sense instead of bulking up the webpack size.
There was a problem hiding this comment.
Interesting! No problem switching packages here, but curious what was the problem with jsonwebtoken to begin with?
Keep in mind all of these packages are separate from admin's npm packages - none of this is deployed/bundled with the app!
There was a problem hiding this comment.
In the sign class in jsonwebtoken it requires KeyObject, createSecretKey, createPublicKey from crypto which it is unable to resolve. The devs claim crypto is unresolvable because it's not intended to be accessed/executed from a browser environment but serverside where node.js is running.
It's odd to me though because we're using node 16 which includes the imports that jsonwebtoken requests from crypto. There seems to be some separation between what Cypress serves up for the browser gui / running tests, and the Cypress backend.
That said we can do some pretty neat stuff with polyfills by customizing Cypress's internal webpack config which I attempted initially with some success. But it resulted in bunch of odd stuff:
- Import paths were different
- The a11y suite completely broke
- Specified imports like
import { LoginPage } from "../../Notify/Admin/Pages/all"got really weird. It refused to import the specific class from the module, resulting in access looking likeLoginPage.default.LoginPage.<someMethod()>
My guess is that we have to pass a webpack config of our own in to Cypress if we enable the webpack preprocessor to do polyfills.
There was a problem hiding this comment.
I see. What was the issue with the JWTs though? They had been working in the rest of the test suite - unless a recent package update broke something?
There was a problem hiding this comment.
Hmm maybe a package update did break it. Whenever I called CreateJWT it failed on the call to jwt.sign() with KeyObject, createSecretKey, createPublicKey all being undefined.
Happy to dig a bit more, it's probably best to continue using jsonwebtoken at it seems to be the go to standard.
| @@ -9,12 +9,12 @@ | |||
| "cypress-recurse": "^1.23.0", | |||
| "html-validate": "^8.7.1", | |||
| "imap-simple": "^5.1.0", | |||
| "jsonwebtoken": "^9.0.0", | |||
| "jsrsasign": "^11.0.0", | |||
There was a problem hiding this comment.
Interesting! No problem switching packages here, but curious what was the problem with jsonwebtoken to begin with?
Keep in mind all of these packages are separate from admin's npm packages - none of this is deployed/bundled with the app!
| }); | ||
|
|
||
| it('Succeeds with a valid form', () => { | ||
| CreateAccountPage.CreateAccount("john doe", "[email protected]", "16132532223", "BestPassword123!"); |
There was a problem hiding this comment.
Good idea on using aliases for this! 🏅
| CreateAccountPage.CreateAccount("john doe", "[email protected]", "16132532223", "BestPassword123!"); | ||
|
|
||
| // Ensure registration email is received and click registration link | ||
| cy.contains('p', 'To complete your registration for GC Notify, please click the link:').should('be.visible'); |
There was a problem hiding this comment.
So this test ensures the email is received, awesome! 🚀
- Pulled the wait for confirmation email code into it's own action and out of CreateAccount - Added nanoid and use it for unique aliases on the email address - Fetch the confirmation link in a more sane way
Summary | Résumé