Skip to content

Frontend/auth integration#161

Merged
isabeleliassen merged 9 commits intocsg-org:developmentfrom
InspiringApps:frontend/auth-integration
Aug 22, 2024
Merged

Frontend/auth integration#161
isabeleliassen merged 9 commits intocsg-org:developmentfrom
InspiringApps:frontend/auth-integration

Conversation

@jsandoval81
Copy link
Collaborator

@jsandoval81 jsandoval81 commented Aug 16, 2024

Requirements list

  • Local environment:
    • .env
      • See webroot README for values
      • Add VUE_APP_COGNITO_REGION
      • Add VUE_APP_COGNITO_AUTH_DOMAIN_STAFF
      • Add VUE_APP_COGNITO_CLIENT_ID_STAFF

Description List

  • Added Login / Logout pages
    • These are mainly logic & redirects to AWS hosted UI
  • Added AuthCallback page
    • To handle last leg of auth code flow
  • Updated the data layer with token-response & refresh handling
  • Added "Logout" handling to the top nav
  • Switched backend API calls from /mock to authenticated /v0
  • Updated frontend README with .env vars
  • Updated backend lambda function with CSP allowances
  • Updated GitHub workflow files with new frontend .env vars

Draft Todo

Closes #109

@jlkravitz
Copy link
Collaborator

jlkravitz commented Aug 20, 2024

  • review the pull request to get oriented
    • read the description of the pull request, which should summarize the changes made
    • read through every task on the Scrum board that's encompassed by this pull request
    • read the description of the commits that comprise the pull request
  • skim all new code, in the context of existing code, looking for problems (knowing that the vast majority of new code will be covered by tests)
  • review all tests
    • methodically review all new tests for correctness, quality of naming
    • look at code coverage of tests
    • determine what code isn’t tested, review that rigorously
  • review documentation to ensure that it matches changes
  • provide comments on the pull request on GitHub, as necessary
    • for comments that are specific to a particular line of code, comment on those specific lines
    • for comments that are more general, attach the comment to a random line in README.md (as opposed to commenting on the pull request itself), to be able to use GitHub's ability to thread discussions on those comments
  • run a security audit of dependencies (e.g. npm audit and pip audit) to ensure that there are no vulnerabilities that will be deployed to production (as opposed to vulnerabilities that only have an impact on the development environment)

Copy link
Collaborator

@jlkravitz jlkravitz left a comment

Choose a reason for hiding this comment

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

Looks good! A few comments, but generally all makes sense to me. Thanks for the thorough testing.

Copy link
Collaborator

@jlkravitz jlkravitz left a comment

Choose a reason for hiding this comment

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

Missed one question, and apparently I can't edit my prior review?

Copy link
Collaborator

@jlkravitz jlkravitz left a comment

Choose a reason for hiding this comment

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

Any reason the test suite throws this error?

Error: AggregateError
    at Object.dispatchError (/Users/joshua/Documents/CompactConnect/webroot/node_modules/jsdom/lib/jsdom/living/xhr/xhr-utils.js:63:19)
    at Request.<anonymous> (/Users/joshua/Documents/CompactConnect/webroot/node_modules/jsdom/lib/jsdom/living/xhr/XMLHttpRequest-impl.js:655:18)
    at Request.emit (node:events:532:35)
    at ClientRequest.<anonymous> (/Users/joshua/Documents/CompactConnect/webroot/node_modules/jsdom/lib/jsdom/living/helpers/http-request.js:121:14)
    at ClientRequest.emit (node:events:520:28)
    at Socket.socketErrorListener (node:_http_client:502:9)
    at Socket.emit (node:events:520:28)
    at emitErrorNT (node:internal/streams/destroy:170:8)
    at emitErrorCloseNT (node:internal/streams/destroy:129:3)
    at processTicksAndRejections (node:internal/process/task_queues:82:21)

@jsandoval81
Copy link
Collaborator Author

@jlkravitz

Any reason the test suite throws this error?

I'm not 100% sure, but that seems like a few errors bubbling up from a socket listener. It may be that you reached some socket / port limit locally, or that local networking just got gummed up. Is it happening regularly? I'm not able to reproduce with the standard test command.

@jsandoval81 jsandoval81 marked this pull request as ready for review August 21, 2024 15:21
@jlkravitz
Copy link
Collaborator

@jlkravitz

Any reason the test suite throws this error?

I'm not 100% sure, but that seems like a few errors bubbling up from a socket listener. It may be that you reached some socket / port limit locally, or that local networking just got gummed up. Is it happening regularly? I'm not able to reproduce with the standard test command.

@jsandoval81 It shows up on the Github Action output, and when I run locally. I agree it's something bubbling up, and also not sure! Let's keep an eye on it but not high priority.

@jlkravitz
Copy link
Collaborator

@isabeleliassen This PR is good to merge. I will do a final, end-of-sprint review once this is merged.

@jsandoval81 jsandoval81 requested a review from jlkravitz August 21, 2024 22:08
@isabeleliassen isabeleliassen merged commit 5141e6b into csg-org:development Aug 22, 2024
@jsandoval81 jsandoval81 deleted the frontend/auth-integration branch November 20, 2024 17:18
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.

staff users frontend authentication implementation

3 participants