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

Clarify local development login flow #2540

Merged
merged 13 commits into from
Feb 2, 2024
Merged
7 changes: 5 additions & 2 deletions docker/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,11 @@ steps to prepare your environment:
4. Run `docker compose up -d` to start the services in the background (the `-d` flag).
5. Install application dependencies via yarn: `docker compose exec app yarn`.
6. Visit http://localhost:8080 to confirm you see 'Grants Identification Tool' with a prompt to enter an email address.
If you see a blank screen, [review the logs](#cookbook-logs), you may need to [run a db migrate](#cookbook-db-migrate) and restart the app container.

- If you see a blank screen, [review the logs](#cookbook-logs), you may need to [run a db migrate](#cookbook-db-migrate) and restart the app container.
7. To enable logging in, seed the DB with data and use one of the generic login emails
- Run the DB seed script: `docker compose exec app yarn db:seed`
- At the login page, enter either `admin@example.com` or `staff@example.com`
- Check the logs for a generated login URL `docker compose logs -f app` and open it!

**Note:** Some systems may have Docker Compose installed as an integrated plugin for Docker,
while others may have it installed as a standalone executable (e.g. via `pip install`).
Expand Down
3 changes: 3 additions & 0 deletions docs/setup-gmail.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# GMail Setup

> [!WARNING]
> Email setup is generally not required for local development, unless you're directly working on email templates or sending. Note that with this setup you will send real emails — please ensure you don't have real external email addresses in your database that you could accidentally mail. Please revert these environment variables to disable email sending anytime you're not actively developing email.

Comment on lines +3 to +5
Copy link
Member

Choose a reason for hiding this comment

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

❤️

Users log into the app by means of a single-use link that is sent to their email. In order to set your app up to send this email, you'll need to setup an App Password in Gmail.

Visit: <https://myaccount.google.com/apppasswords> and set up an "App Password" (see screenshot below). *Note: Select "Mac" even if you're not using a Mac.*
Expand Down
4 changes: 2 additions & 2 deletions packages/server/__tests__/api/agencies.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ describe('`/api/organizations/:organizationId/agencies` endpoint', () => {
before(async () => {
admin = await createUser({
name: 'Test Admin',
email: 'admin@example.com',
email: 'adminuser@example.com',
agency_id: 0,
tenant_id: 1,
role_id: fixtures.roles.adminRole.id,
Expand Down Expand Up @@ -130,7 +130,7 @@ describe('`/api/organizations/:organizationId/agencies` endpoint', () => {
it('issues 400 Bad Request when agency has users', async () => {
const blockingUser = await createUser({
name: 'Some One',
email: 'staff@example.com',
email: 'staffuser@example.com',
agency_id: agency.id,
tenant_id: agency.tenant_id,
role_id: fixtures.roles.staffRole.id,
Expand Down
2 changes: 1 addition & 1 deletion packages/server/__tests__/api/users.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ describe('`/api/users` endpoint', () => {
const response = await fetchApi(`/users`, agencies.own, fetchOptions.admin);
expect(response.statusText).to.equal('OK');
const json = await response.json();
expect(json.length).to.equal(12);
expect(json.length).to.equal(14);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It feels like ideally these tests would be independent of the seed data. Does that track? (Regardless, seems outside the scope of this change, so I'm just updating the test to pass.)

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Maybe something like this instead?

Suggested change
expect(json.length).to.equal(14);
const ownAgency = await knex('agencies').where({ id: agencies.own }).first();
const expectedUserCount = (
await knex('users').where({ tenant_id: ownAgency.tenant_id }).count().first()
).count;
expect(json.length).to.equal(parseInt(expectedUserCount, 10));

Note: To make the above work, you'll also have to modify line 3 to import knex from ./utils, e.g.

const { getSessionCookie, makeTestServer, knex } = require('./utils');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this seems like a fair approach for now! (I'm curious, have you explored using synthetic test data generated by the tests themselves, as opposed to using common seed data? Any docs/discussions I can read up on?)

});
it('lists users for an agency outside this user\'s hierarchy but in the same tenant', async () => {
const response = await fetchApi(`/users`, agencies.offLimits, fetchOptions.admin);
Expand Down
51 changes: 35 additions & 16 deletions packages/server/seeds/dev/ref/users.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ const agencies = require('./agencies');
const roles = require('./roles');
const tenants = require('./tenants');

const adminRole = roles.find(({ name }) => name === 'admin');
const staffRole = roles.find(({ name }) => name === 'staff');

const usdrAgency = agencies.find((a) => a.abbreviation === 'USDR');
const usdrSubAgency = agencies.find((a) => a.abbreviation === 'TSDR');
const nevadaAgency = agencies.find((a) => a.abbreviation === 'NV');
Expand All @@ -21,127 +24,143 @@ module.exports = [
email: 'alex@usdigitalresponse.org',
name: 'Alex Allain',
agency_id: usdrAgency.id,
role_id: roles[0].id,
role_id: adminRole.id,
tenant_id: usdrTenant.id,
},
{
id: 2,
email: 'mindy@usdigitalresponse.org', // fake email for testing
name: 'Mindy Huang',
agency_id: usdrAgency.id,
role_id: roles[0].id,
role_id: adminRole.id,
tenant_id: usdrTenant.id,
},
{
id: 3,
email: 'joecomeau01@gmail.com',
name: 'Joe Comeau',
agency_id: usdrAgency.id,
role_id: roles[0].id,
role_id: adminRole.id,
tenant_id: usdrTenant.id,
},
{
id: 4,
email: 'asridhar@usdigitalresponse.org',
name: 'Aditya Sridhar',
agency_id: usdrAgency.id,
role_id: roles[0].id,
role_id: adminRole.id,
tenant_id: usdrTenant.id,
},
{
id: 5,
email: 'thendrickson@usdigitalresponse.org',
name: 'Tyler Hendrickson',
agency_id: usdrAgency.id,
role_id: roles[0].id,
role_id: adminRole.id,
tenant_id: usdrTenant.id,
},
{
id: 6,
email: 'user1@nv.gov', // fake email for testing
name: 'nv.gov User 1',
agency_id: nevadaAgency.id,
role_id: roles[1].id,
role_id: staffRole.id,
tenant_id: nevadaenant.id,
},
{
id: 7,
email: 'user2@nv.gov', // fake email for testing
name: 'nv.gov User 2',
agency_id: nevadaAgency.id,
role_id: roles[1].id,
role_id: staffRole.id,
tenant_id: nevadaenant.id,
},
{
id: 8,
email: 'user3@nv.gov', // fake email for testing
name: 'nv.gov User 3',
agency_id: nevadaAgency.id,
role_id: roles[1].id,
role_id: staffRole.id,
tenant_id: nevadaenant.id,
},
{
id: 9,
email: 'user1@npo.nv.gov', // fake email for testing
name: 'npo.nv.gov User 1',
agency_id: procurementAgency.id,
role_id: roles[1].id,
role_id: staffRole.id,
tenant_id: procurementTenant.id,
},
{
id: 10,
email: 'user2@npo.nv.gov', // fake email for testing
name: 'npo.nv.gov User 2',
agency_id: procurementAgency.id,
role_id: roles[1].id,
role_id: staffRole.id,
tenant_id: procurementTenant.id,
},
{
id: 11,
email: 'user3@npo.nv.gov', // fake email for testing
name: 'npo.nv.gov User 3',
agency_id: procurementAgency.id,
role_id: roles[1].id,
role_id: staffRole.id,
tenant_id: procurementTenant.id,
},
{
id: 12,
email: 'user1@dallas.gov', // fake email for testing
name: 'dallas.gov User 1',
agency_id: dallasAgency.id,
role_id: roles[0].id,
role_id: adminRole.id,
tenant_id: dallasTenant.id,
},
{
id: 13,
email: 'admin1@nv.gov', // fake email for testing
name: 'nv.gov Admin User 1',
agency_id: nevadaAgency.id,
role_id: roles[0].id,
role_id: adminRole.id,
tenant_id: nevadaenant.id,
},
{
id: 14,
email: 'mindy+testsub@usdigitalresponse.org',
name: 'USDR tenant sub agency user',
agency_id: usdrSubAgency.id,
role_id: roles[1].id,
role_id: staffRole.id,
tenant_id: usdrTenant.id,
},
{
id: 15,
email: 'nat.hillard.usdr@gmail.com',
name: 'Nat Hillard',
agency_id: usdrSubAgency.id,
role_id: roles[0].id,
role_id: adminRole.id,
tenant_id: usdrTenant.id,
},
{
id: 16,
email: 'admin1@usdigitalresponse.org',
name: 'USDR tenant sub agency admin',
agency_id: usdrSubAgency.id,
role_id: roles[1].id,
role_id: staffRole.id,
tenant_id: usdrTenant.id,
},
{
id: 17,
email: 'admin@example.com',
name: 'USDR Admin',
agency_id: usdrAgency.id,
role_id: adminRole.id,
tenant_id: usdrTenant.id,
},
{
id: 18,
email: 'staff@example.com',
name: 'USDR Staff',
agency_id: usdrAgency.id,
role_id: staffRole.id,
tenant_id: usdrTenant.id,
},
];
6 changes: 5 additions & 1 deletion packages/server/src/lib/email.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,11 @@ function sendPassCode(email, passcode, httpOrigin, redirectTo) {
);

if (process.env.DEV_LOGIN_LINK && process.env.NODE_ENV === 'development') {
console.log(`Login link generated: \x1b[32m${href}`);
const BLUE = '\x1b[34m';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if the preference was for something like this vs using a library like chalk. We seem to have chalk installed but only as a dev dependency, so I didn't want to rely on it here, but I'm happy to update if that feels cleaner.

Copy link
Member

Choose a reason for hiding this comment

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

🤔 Given that this execution path is specific to dev workflows, is there a way to use chalk without declaring it as a production dependency?

In general, my preference for logging is to slowly migrate over to structured logging (we currently use bunyan for this, with a preconfigured base logger exported by lib/logging.js). However, given that this is specifically intended to be human-readable and not something we would try to query with a tool like Datadog, it may be best to leave the colorization as-is for now (assuming there isn't a way to use chalk here without bringing it in as a prod dependency).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible to use chalk here from devDependencies, but I think it ends up more messy than worth it. You run into a few issues you have to skirt:

  1. The airbnb base eslint config we use disallows devDependencies being used outside test and config files (https://github.com/airbnb/javascript/blob/master/packages/eslint-config-airbnb-base/rules/imports.js#L71-L95), so we'd have to either change our eslint config or ignore the rule for this line of code.
  2. We're using chalk v5, which moved to ESM, meaning we can't require('chalk') without downgrading to v4 or using a pretty hacky-feeling dynamic import workaround.

Here's basically what that code would look like:

    if (process.env.DEV_LOGIN_LINK && process.env.NODE_ENV === 'development') {
        // eslint-disable-next-line import/no-extraneous-dependencies
        import('chalk').then((chalkModule) => {
            const chalk = chalkModule.default;
            const message = `| Login link generated: ${href} |`;
            console.log(chalk.blue('-'.repeat(message.length)));
            console.log(chalk.blue(message));
            console.log(chalk.blue('-'.repeat(message.length)));
        });
    }

Personally, I don't think it's a particular improvement. It also has the small danger of someone copy-pasting code that relies on chalk outside of the NODE_ENV === 'development' check and possibly breaking staging/prod.

If this is the only instance of wanting color output for now, I'd be inclined to leave it as-is. If we start needing this a few places, I'd probably consider promoting chalk to a full dependency. Since this is server-side code, we shouldn't be as worried about bundle size, etc.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Agree with the above – let's leave as-is unless/until there are any compelling use-cases for production.

const message = `| Login link generated: ${href} |`;
console.log(`${BLUE}${'-'.repeat(message.length)}`);
console.log(`${BLUE}${message}`);
console.log(`${BLUE}${'-'.repeat(message.length)}`);
}
return module.exports.deliverEmail({
toAddress: email,
Expand Down
Loading