GEOLYTIX / xyz

An open source javascript framework for spatial data and application interfaces.
MIT License
86 stars 25 forks source link

Review _user API mod #1264

Closed dbauszus-glx closed 1 week ago

dbauszus-glx commented 1 month ago

The _user API methods should each individually check whether the ACL is provided and user admin rights are required.

The admin view method was pushed into it's own mod file user/admin.js

The user/delete API will now allow for a logged in user to remove themselves.

RobAndrewHurst commented 4 weeks ago

I am happy for this to be reviewed. @simon-leech Do you still want to write some tests for this? Or should I try give it a crack?

simon-leech commented 4 weeks ago

@RobAndrewHurst Happy for you to pick up the tests then I can review the PR and tests all in one go?

RobAndrewHurst commented 4 weeks ago

Sure!

RobAndrewHurst commented 4 weeks ago

@RobAndrewHurst Happy for you to pick up the tests then I can review the PR and tests all in one go?

The main problem with testing these modules is that we can't test them via the CLI. We would want to write a login_test similar to the test_view to the run on deployed instances. This will then fully test the ACL.

I don't see a point in mocking the ACL in the CLI.

Let me know what you think?

@AlexanderGeere @dbauszus-glx @simon-leech

simon-leech commented 4 weeks ago

@RobAndrewHurst Happy for you to pick up the tests then I can review the PR and tests all in one go?

The main problem with testing these modules is that we can't test them via the CLI. We would want to write a login_test similar to the test_view to the run on deployed instances. This will then fully test the ACL.

I don't see a point in mocking the ACL in the CLI.

Let me know what you think?

@AlexanderGeere @dbauszus-glx @simon-leech

@RobAndrewHurst Happy for you to pick up the tests then I can review the PR and tests all in one go?

The main problem with testing these modules is that we can't test them via the CLI. We would want to write a login_test similar to the test_view to the run on deployed instances. This will then fully test the ACL.

I don't see a point in mocking the ACL in the CLI.

Let me know what you think?

@AlexanderGeere @dbauszus-glx @simon-leech

@RobAndrewHurst Agreed - sounds quite tricky to mock, maybe we can add a login_test view as a separate PR? Once this one is in?

RobAndrewHurst commented 4 weeks ago

@RobAndrewHurst Agreed - sounds quite tricky to mock, maybe we can add a login_test view as a separate PR? Once this one is in?

Sounds good!

dbauszus-glx commented 4 weeks ago

This is back into draft. There was a couple of issues especially in regards to the auth mod, token and session check.

The session check did not work when no session field is available in ACL. This was failing silently with the request authenticating.

The session and token check are now individual documented methods.

dbauszus-glx commented 3 weeks ago

This should now be ready for review.

I created a separate issue for the api mod review and documentation as this is a bigger job.

sonarcloud[bot] commented 3 weeks ago

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

dbauszus-glx commented 1 week ago

The update method as called from the admin panel will now do the verification bits.

  let verification_by_admin = ''
  if (req.params.field === 'verified' && req.params.value === true) {

    verification_by_admin = `
      , password = password_reset
      , password_reset = NULL
      , failedattempts = 0
      , verificationtoken = NULL
      , approved = true
      , approved_by = '${req.params.user.email}|${ISODate}'
    `
  }

  // Get user to update from ACL.
  const rows = await acl(`
    UPDATE acl_schema.acl_table
    SET
      ${req.params.field} = $2
      ${verification_by_admin}
      ${approved_by}
    WHERE lower(email) = lower($1);`,
    [email, req.params.value])

  if (rows instanceof Error) {
    return res.status(500).send('Failed to access ACL.')
  }

The verification can be called directly by an admin.

http://localhost:3000/api/user/update?email=dennis@mail.com&field=verified&value=true
dbauszus-glx commented 1 week ago

@RobAndrewHurst No, we cannot. Otherwise I could set your verification to false knowing your email address.

Anybody could... I can anyways being an administrator. But you get what I mean.

RobAndrewHurst commented 1 week ago

We then need a way for Admin's to easily see who needs to be verified again

dbauszus-glx commented 1 week ago

I added a column re_verification which is false if there user [record] has a verification token. If clicked on this the the verified true update query will be sent and the verified and approved columns will be toggled after successful update query.

RobAndrewHurst commented 1 week ago

image

Resetting a users email (roberthurstsa@gmail.com) I don't see a green tick in the outstanding password reset column.

But after re-verifying the user through the admin panel, I then see a green tick for the user. Seems as if the logic needs to be reversed?

image ^^ This is after re-verifying the user.

dbauszus-glx commented 1 week ago

Green tick means that the user is fully verified. Happy for you to reverse the logic.

RobAndrewHurst commented 1 week ago

Green tick means that the user is fully verified. Happy for you to reverse the logic.

Ah ok! If you put it like that I am happy with it!

RobAndrewHurst commented 1 week ago

image

Should we change the message returned from this to maybe "Your session has been terminated. Please login again."

RobAndrewHurst commented 1 week ago

image Maybe something like this.

dbauszus-glx commented 1 week ago

@RobAndrewHurst The issue with missing host should be resolved in last commit.

The register request get's short circuited in the api module since it doesn't have user auth by definition.

Hence the reqHost util is required in the register module itself.

simon-leech commented 1 week ago

It doesn't seem to want me to mark it as approved though idk why 😂

dbauszus-glx commented 1 week ago

The register new user method will no longer create a SQL Select statement but provide a values array to the ACL mod.

  const USER = {
    email: req.body.email,
    password: req.body.password,
    password_reset: req.body.password,
    language: req.body.language,
    verificationtoken: req.body.verificationtoken,
    access_log: [`${date}@${req.ips && req.ips.pop() || req.ip}`]
  }

  if (process.env.APPROVAL_EXPIRY) {
    INSERTS.push('expires_on')
    VALUES.push(expiry_date)
    USER['expires_on'] = expiry_date
  }

  // Create new user account
  const rows = await acl(`
    INSERT INTO acl_schema.acl_table (${Object.keys(USER).join(',')})
    VALUES (${Object.keys(USER).map((NULL,i) => `\$${i+1}`)})`,
    Object.values(USER))

The user admin script will use tabulator 6.2 imports from unpkg.

The user admin cell toggle script will apply the verified logic for verified column clicks as well as re-verification.