Merge pull request #934 from SuffolkLITLab/918_signin

Close #918, detect sign in. Update some deps.

.github/workflows/github_server.yml

@@ -52,6 +52,8 @@ jobs:
       EMPTY_STRING: ""
       USER_NO_PERMISSIONS_EMAIL: alkiln@example.com
       USER_NO_PERMISSIONS_PASSWORD: User123^
+      WRONG_EMAIL=wrong_email@example.com
+      WRONG_PASSWORD=wrong_password
 
     steps:
       # Place the root directory in this branch to access

.github/workflows/playground.yml

@@ -47,6 +47,8 @@ jobs:
       USER_NO_PERMISSIONS_PASSWORD: ${{ secrets.USER_NO_PERMISSIONS_PASSWORD }}
       USER_NO_PERMISSIONS_API_KEY: ${{ secrets.USER_NO_PERMISSIONS_API_KEY }}
       INVALID_API_KEY: invalidAPIkey
+      WRONG_EMAIL=wrong_email@example.com
+      WRONG_PASSWORD=wrong_password
       EMPTY_STRING: ""
       SECRET_VAR1: secret-var1-value
       SECRET_VAR2: secret-var2-value

CHANGELOG.md

@@ -45,9 +45,21 @@ Format:
 
 ## [Unreleased]
 
-## Internal
+### Changed
+
+- Moves "expected" status error to only be visible to internal test errors. Closes [#993](https://github.com/SuffolkLITLab/ALKiln/issues/933).
+
+### Fixed
+
+- Detects failed sign in. Closes [#918](https://github.com/SuffolkLITLab/ALKiln/issues/918).
+
+### Internal
 
-- Check log codes more robustly and flexibly. See [#920](https://github.com/SuffolkLITLab/ALKiln/issues/920).
+- Adds decision docs
+- Checks log codes more robustly and flexibly. See [#920](https://github.com/SuffolkLITLab/ALKiln/issues/920).
+- Returns report text from `.addToReport()` so it can be used again for error messages.
+- Update puppeteer to v22.15.0. Closes [#930](https://github.com/SuffolkLITLab/ALKiln/issues/930).
+- Update npm vulnerabilities except pdfjs-dist. See docs/decisions/pdfjs_version_2024_08_08.md.
 
 ## [5.13.0] - 2024-07-11
 

docassemble/ALKilnTests/data/sources/establishing_steps.feature

@@ -71,3 +71,12 @@ Scenario: I go to a fully arbitrary url
   Successfully went to "https://retractionwatch.com"
   """
   Given I go to "https://retractionwatch.com"
+
+@fast @e10 @signin @failure
+Scenario: I fail to sign in with wrong email and password
+  Given the final Scenario status should be "failed"
+  Given the Scenario report should include:
+  """
+  ALK0208
+  """
+  And I sign in with "WRONG_EMAIL", "WRONG_PASSWORD"

docassemble/ALKilnTests/data/sources/reports.feature

@@ -591,7 +591,7 @@ Scenario: Report lists unused table rows
 Scenario: Sign in to server successfully
   Given the Scenario report should include:
   """
-  Signed in
+  signed in
   """
   Given I sign in with the email "USER1_EMAIL" and the password "USER1_PASSWORD"
 

docs/decisions/adopt_architectural_decision_records.md

@@ -0,0 +1,58 @@
+# Document decisions
+
+## Context and Problem Statement
+
+What is a good way to avoid re-hashing old decisions, or to at least know why those decisions were made?
+
+## Considered Options
+
+- GitHub issues
+- GitHub discussions
+- Actual files in the repo
+
+Formats:
+- https://adr.github.io/madr
+
+
+## Decision Outcome
+
+Use in-repo .md files using the https://adr.github.io/madr format. We found keeping the data within the repo to be the most compelling factor.
+
+## Pros and Cons of the Options
+
+### GitHub issues
+
+Pros:
+
+- Easy to search.
+- Can add labels and such.
+- In one of the most common places to search.
+- Allows comments
+
+Cons:
+
+- Not actually in the repository, so any forks or clones would lose that info.
+- Comments can get confusing and hard to read.
+
+### GitHub discussions
+
+Pros:
+
+- Labels and such.
+- Probably easy to search?
+
+Cons:
+
+- A separate place from issues. Not everyone even knows those exist.
+- All the cons of GitHub issues
+
+### Actual files in the repo
+
+Pros:
+
+- They go everywhere the repo goes.
+
+Cons:
+
+- Probably not as searchable, though maybe we could institute a way to "label"/tag them.
+- Not very visible.

docs/decisions/detect_sign_in.md

@@ -0,0 +1,124 @@
+# Use response data to detect sign-in success
+
+## Context and Problem Statement
+
+What is a robust way to detect whether signing in worked or not?
+
+## Considered Options
+
+- Elements in the DOM.
+  - Use some class or ID that only the sign-in page has, so we can detect that a user is on a new page and succeeded.
+  - User-facing invalidation message elements in the DOM on failure.
+- URL change.
+- Response data.
+
+## Decision Outcome
+
+Use a change in the URL to detect sign-in navigation.
+
+## Pros and Cons of the Options
+
+### DOM
+
+Pros:
+
+- It can be easy to do with the tools we are using.
+
+Cons:
+
+- The DOM can, and has been, changed by docassemble and these kinds of changes are made pretty quietly so we find it hard to keep track of them.
+- It is pretty easy for site developers to change the HTML of standard pages like the sign-in page.
+
+### URL change
+
+Pros:
+
+- It seems very likely that developers will keep an unsuccessful sign-in on the same page, even if they change some other functionality.
+- It seems very likely that developers will change the url when the sign-in succeeds.
+- Sticking with checking the url would mesh with how we currently detecting page navigation. See the cons for issues with that.
+
+Cons:
+
+- It seems precarious, though I haven't yet been able to articulate why. I know it hasn't been a sufficient full solution in the past for interview pages. For example, in a `show if` infinite page loop.
+- Developers can manipulate a page's url, though it seems unlikely in this situation.
+
+### Response data
+
+Use the response data from the sign in request.
+
+Pros:
+
+- It seems reliable - it seems very likely that developers will re-direct their users to a different page, even if they change some other functionality.
+- Less likely to change than the DOM.
+- Has some standard values we can expect, like status codes.
+
+Cons:
+
+- DA's responses are not typical. There's no 400's in unsuccessful responses.
+- Regular interview page navigation doesn't get a 302, so this method would be a special case for signing in.
+
+Options are:
+
+- 302 redirection status. It seems fairly reliable that a developer will send a user to a different page after they sign in, and also likely that they will keep the user on the same page if the sign-in failed.
+- Cache-Control header value that contains "must-revalidate" (See [MDN cache control directives](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#directives)). It makes sure this data isn't cached. I'm not familiar with this and I'm not sure what might or might not trigger this.
+- Cookie with the name "secret". This seems promising, but I don't know much about cookies.
+
+Based on our current knowledge, we think the 302 status response will be most reliable.
+
+Notes for response data:
+
+I have isolated the differences between the sign-in success and failure responses.
+
+Key for following entries:
+
+```
+<key of object containing the data>
+  🌈 <sign in success data>
+vs
+  🤕 <sign in failure data>
+```
+
+Findings:
+
+```
+response
+  🌈 "redirectURL": "/path", "headersSize": 552, "bodySize": 0, "_transferSize": 552,
+vs
+  🤕 "redirectURL": "", "headersSize": 570, "bodySize": 1998, "_transferSize": 2568,
+---
+response
+  🌈 "status": 302, "statusText": "FOUND",
+vs
+  🤕 "status": 200, "statusText": "OK",
+
+---
+headers
+  🌈 no cache-control
+vs
+  🤕 { "name": "Cache-Control", "value": "no-store, no-cache, must-revalidate, post-check=0, pre-check=0, max-age=0" },
+---
+headers
+  🌈 { "name": "Content-Length", "value": "209" },
+vs
+  🤕 { "name": "Content-Encoding", "value": "gzip" },
+---
+headers
+  🌈 { "name": "Location", "value": "/interviews" },
+vs
+  🤕 no location
+---
+headers
+  🌈 { "name": "Set-Cookie", "value": "secret=secretchars; Secure; HttpOnly; Path=/; SameSite=Lax" },
+vs
+  🤕 no secret
+---
+headers
+  🌈 no transfer-encoding
+vs
+  🤕 { "name": "Transfer-Encoding", "value": "chunked" },
+---
+cookies
+  🌈 { "name": "secret", "value": "secretchars", "path": "/", "domain": "site.com", "expires": null, "httpOnly": true, "secure": true, "sameSite": "Lax" },
+vs
+  🤕 no secret
+```

docs/decisions/pdfjs_version_2024_08_08.md

@@ -0,0 +1,18 @@
+# Leave pdfjs at its current version
+
+## Context and Problem Statement
+
+pdfjs has a security vulnerability (that does not affect us) where malicious PDFs can inject code into an unsuspecting project and cause havoc.
+
+Our users are downloading and manipulating their own PDFs. We will assume they're not putting malicious code into their PDFs.
+
+The update changes pdfjs .js files to be .mjs files. It might take some bother to configure our repo to use .mjs files.
+
+## Considered Options
+
+- Update pdfjs
+- Leave pdfjs at its current version, 3.2.146
+
+## Decision Outcome
+
+Leave pdfjs at its current version, 3.2.146. The vulnerability is irrelevant for our situation and we don't have bandwidth to set up the configurations right now.

docs/decisions/update_puppeteer_2024_07_28.md

@@ -0,0 +1,62 @@
+# Use response data to detect sign-in success
+
+## Context and Problem Statement
+
+Should we update to the newest version of puppeteer, [22.15.0](https://github.com/puppeteer/puppeteer/blob/main/packages/puppeteer-core/CHANGELOG.md#22150-2024-07-31)?
+
+See https://github.com/SuffolkLITLab/ALKiln/issues/930.
+
+## Considered Options
+
+- Update to 22.15.0
+- Update to 22.12.0, which is the lowest version that would fix our current problems
+- Stay with 20.8.2
+
+See pros and cons
+
+## Decision Outcome
+
+Update puppeteer from 20.8.2 to 22.15.0
+
+## Pros and Cons of the Options
+
+### Update at all {#update}
+
+**Pros:**
+
+- Makes the current fix possible
+
+**Cons:**
+
+- Change in the middle of a complex PR
+- [A few breaking changes](https://github.com/puppeteer/puppeteer/blob/main/packages/puppeteer-core/CHANGELOG.md#2200-2024-02-05), which includes for us at the very least:
+   - "Removes the deprecated `$x` (replace with `$$`) and `waitForXpath` (replace with `waitForSelector`)." We need to add [extra syntax to the start of our selector strings](https://github.com/puppeteer/puppeteer/pull/11782): "xpath//."
+   - Replace any `page.waitForTimeout` with cucumber's version or ours.
+
+### Update to to 22.15.0
+
+**Pros:**
+
+- Has all the most recent bug fixes and features
+
+**Cons:**
+
+- Exposes us to possible unknown new bugs
+
+
+### Update to to 22.12.0
+
+This is the lowest version that would fix our problem
+
+**Pros:**
+
+- Avoids possibly unknown new bugs
+
+**Cons:**
+
+- Doesn't have fixes to already known bugs
+
+
+### Stay with 20.8.2
+
+Opposite of first entry ["Update at all"](#update).