digitalbazaar / bedrock-account-http

HTTP APIs for Bedrock User Accounts

bedrock-account-http

HTTP APIs for Bedrock User Accounts

bedrock-account-http's Issues

Replace `req.logIn` with promise version from passport once available

Upgrade peerDependencies to latest and upgrade tests to MongoDb 4.2

Many peerDepedencies of this package have recently received a major release.

In particular peerDepedencies will need to follow this pattern lastRelease.x - latestRelease.x.

Documentation on peerDeps can be found here: https://nodejs.org/en/blog/npm/peer-dependencies/

The latest major releases are:

bedrock-kms: ^3.0.0
bedrock-edv-storage: ^3.0.0
bedrock-session-mongodb: ^4.0.0
bedrock-identity: ^8.0.0
bedrock-vc-issuer: ^4.0.0
bedrock-ssm-mongodb: ^3.0.0
bedrock-zcap-storage: ^3.0.0
bedrock-authn-token: ^2.0.0
bedrock-profile: ^5.00
bedrock-account: ^3.0.0
bedrock-permission: ^3.0.0

You will also need to upgrade the test project.
This should require 2 steps:

1. Update the test dependencies to the latest releases.
2. Change the .github/workflows/main.yml so that MongoDb is version 4.2

This Draft PR can be used a model: digitalbazaar/bedrock-kms-http#25

Add Tests that Properly use Roles

Current Tests just set permissions directly via a stub of the passport method that sets req.user.
We want to remove that make sure the actor set by the stub actually really has access to the account.

• insert an identity for an actor that has ACCOUNT_ACCESS permission

Add validation

Consider refactoring registration authorization as a middleware

          Moving this out for later refactoring and providing some context around what the "token" is and what it's for:

      if(cfg.registration.authorizationRequired.length > 0) {
        await _authorizeRegistration({req});
      }

Down below where helpers are defined:

async function _authorizeRegistration({req}) {
  const cfg = config['account-http'];
  const {authorizationRequired} = cfg.registration;
  const {authorization} = req.body;
  if(!(authorization?.type === authorizationRequired &&
    authorization.token)) {
    throw new BedrockError(
      `Authorization using "${authorizationRequired}" is required.`, {
        name: 'NotAllowedError',
        details: {
          httpStatusCode: 403,
          public: true
        }
    });
  }
  
  const remoteIp = req.socket.remoteAddress;
  await _verifyTurnstileToken({token: authorization.token, remoteIp});
}

Since the above fits the middleware pattern, I'll file an issue about potentially making this a middleware in the future.

I'm also not sure if using req.socket.remoteAddress is will work in deployment scenarios -- we'll need to figure that out before merging the PR to make sure we're pulling the IP from the right place.

Originally posted by @dlongley in #42 (comment)

Update peer dep to include bedrock-passport@6

https://github.com/digitalbazaar/bedrock-account-http/blob/master/package.json#L35

Upgrade to bedrock-account@4 and ensure no links to bedrock-identity.

Add the ability to find all accounts associated with an email address

We need the ability to search for accounts by email address. This in turn requires the following features in bedrock-account-http:

1. create a pagination object.

this should help:

https://github.com/digitalbazaar/p3/blob/master/lib/payswarm-auth/services.transaction.js#L730

https://www.dataversity.net/introduction-linked-data-platform/

Admins should be able create a password with a code

This comes from the spec the expected feature is:

Add user account

• Email
• Temporary code that is emailed to user

currently an admin can create an account by posting an email address to the basePath

this results in a bedrock-account emitting an account.insert event.

My proposal for this feature is:

1. add a query param to the route temp
2. if temp then we need to check permissions are admin ACCOUNT_INSERT
3. if we pass the permissions check then insert the account
4. add password to the account
5. on successful creation email the user (do we have an email solution like send grid? mail chimp?)
6. return 201

the alternative would be to handle this somewhere else via the bedrock account.insert event.

Update [email protected]

• Update findOne projection calls if any

getAll queries should be able to search by more than email.

The google Docs for getAll specified only search by email.
@mattcollier @gannan08 both want to expand this search functionality.
even the wireframe expects search by more than just email.
so we could refactor:

1. return the email check to exists query and to the current getAll query
2. allow searches by account.meta created,
3. allow searches by status (and filter by status)
4. allow search by sysResourceRole

in the future we could expand to search by credentials / persona
this would allow search by phone number, last name, first name, etc.

Negative tests for registration authz

We are adding optional captcha-style authz for account registration -- and we should ensure we have negative tests for this. Negative tests should include both missing and invalid authz params in a request. This includes ensuring that token verification will fail for a bad token that is otherwise well formed.

TODO/FIXME Count 6

• TODO extend mocha should with this
  Commit: (2bdc2dc) Add validation and tests for all working routes.
  File: test/mocha/10-api.js:30
  Andrew Jones commented a year ago

• TODO: next
  Commit: (64213dc) Add core files.
  File: lib/index.js:225
  Dave Longley commented 2 years ago

• TODO: improve error details
  Commit: (2bdc2dc) Add validation and tests for all working routes.
  File: lib/index.js:117
  Andrew Jones commented a year ago

• FIXME add an implementation in bedrock-express
  Commit: (2bdc2dc) Add validation and tests for all working routes.
  File: lib/index.js:92
  Andrew Jones commented a year ago

• TODO: if passport supports promises or this can be safely promisified
  Commit: (c637816) Add TODO.
  File: lib/index.js:71
  Dave Longley commented 3 months ago

• FIXME add an implementation in bedrock-express
  Commit: (2bdc2dc) Add validation and tests for all working routes.
  File: lib/index.js:17
  Andrew Jones commented a year ago

Add test suite

Actor should default to `undefined`, not `null`

Looks like a copy-paste error that defaults actor to null here:

https://github.com/digitalbazaar/bedrock-account-http/blob/master/lib/index.js#L96

That will prevent permission checks from being run -- the null default should be removed so actor will be undefined, causing the permission check to fail, as it should, when someone tries to read an account without authenticating.

getAll route returns password and salt from account

Testing out the route in actual Vue and found that it returns password sha-256 etc.