Organisation admin role

[nrjadkry]

What type of PR is this? (check all applicable)

• 🍕 Feature
• 🐛 Bug Fix
• 📝 Documentation Update
• 🎨 Style
• 🧑‍💻 Code Refactor
• 🔥 Performance Improvements
• ✅ Test
• 🤖 Build
• 🔁 CI
• 📦 Chore (Release)
• ⏩ Revert

Describe this PR

This PR includes following features related to organisation admin:

• An endpoint to add and remove organisation admin (can only be done by org admin)
• Approval of organisation creation request ( can only be done by super admin)
  For this, a boolean field is added in the organisation table (approved). It will be false by default.
  Then, the admin can view the organisation list that are pending for approval. They will appear in the frontend only
  when they are approved.
• Organisation list endpoint is updated. Admin can view the approved and unapproved organisations. Other users
  can only view the organisations that have been approved by admin
• Endpoint to approve the organisation by admin

Todo

• Add authentication in frontend for organisations list

Checklist before requesting a review

• 📖 Read the FMTM Contributing Guide: https://github.com/hotosm/fmtm/blob/main/CONTRIBUTING.md
• 📖 Read the FMTM Code of Conduct: https://github.com/hotosm/fmtm/blob/main/CODE_OF_CONDUCT.md
• 👷‍♀️ Create small PRs. In most cases, this will be possible.
• ✅ Provide tests for your changes.
• 📝 Use descriptive commit messages.
• 📗 Update any related documentation and include any relevant screenshots.

[nrjadkry]

Tests have failed in this PR.
Looks like migration file is not accepted by github action. @spwoodcock
Also, I have a small query related to the roles function you have made,

async def super_admin(
    db: Session = Depends(get_db),
    user_data: AuthUser = Depends(login_required),
) -> AuthUser:
    """Super admin role, with access to all endpoints."""
    user_id = await get_uid(user_data)

    match = db.query(DbUser).filter_by(id=user_id, role=UserRole.ADMIN).first()

    if not match:
        log.error(f"User ID {user_id} requested an admin endpoint, but is not admin")
        raise HTTPException(
            status_code=HTTPStatus.FORBIDDEN, detail="User must be an administrator"
        )
    return user_data

This is a function made to check if the user is a super_user or not, I guess it would be better if we return boolean value instead of returning exception. So, that we can use this function more effectively. What do you think @spwoodcock

[spwoodcock]

Nice work!

Let's combine the migrations together from #1123

I will update this PR to include the extra fields.

The reason the tests fail if a migration is included, is because the backend ci image is cached with the old migrations.
The way to fix it is to delete the cache and allow it to rebuild: https://github.com/hotosm/fmtm/actions/caches
(the cache is image-cache-Linux for the development branch)

[spwoodcock]

I merged the migrations files into one. Hope the workflow runs 🤞

[spwoodcock]

Tests have failed in this PR. Looks like migration file is not accepted by github action. @spwoodcock Also, I have a small query related to the roles function you have made,

async def super_admin(
    db: Session = Depends(get_db),
    user_data: AuthUser = Depends(login_required),
) -> AuthUser:
    """Super admin role, with access to all endpoints."""
    user_id = await get_uid(user_data)

    match = db.query(DbUser).filter_by(id=user_id, role=UserRole.ADMIN).first()

    if not match:
        log.error(f"User ID {user_id} requested an admin endpoint, but is not admin")
        raise HTTPException(
            status_code=HTTPStatus.FORBIDDEN, detail="User must be an administrator"
        )
    return user_data

This is a function made to check if the user is a super_user or not, I guess it would be better if we return boolean value instead of returning exception. So, that we can use this function more effectively. What do you think @spwoodcock

It's not just a function to determine if the user is admin, it's a dependency.
So this means it will return the object that we want to use, in this case the user object:
user = Depends(super_admin)

This will either return a user if they are admin, or throw an error if they are not.

Doing it this way prevents an extra layer of logic:

is_admin = super_admin(user_id)
if is_admin:
    do_stuff_if_admin

do_stuff_if_not_admin

[spwoodcock]

A couple of things of note:

• Please reset your branch to the latest, to prevent conflicts from the organisation file names: organization_routes --> organisation_routes

  git fetch origin/feat-user-roles
  git reset --hard origin/feat-user-roles

• See my comment above about usage of Depends.
• Please try to use the British English spelling of organisation going forward, for consistency in the code (this was done in PR Pedantic renaming for consistency with database #1114 to avoid conflicts with the already existing database).

[spwoodcock]

The org_admin and super_admin endpoints are special cases, as they use different tables:

• org_admin uses organisation_managers table.
• super_admin uses user.role field.

For all other roles, they are in the user_role table, so we can do an easy hierarchy of roles to determine if the user has permission.
E.g. using > operators to determine that if the MAPPER role is required, then any role > ProjectRoles.MAPPER are accepted.

[spwoodcock]

I see the issue you were facing:

• In most cases endpoints are assessing the role based on the user that called them.
• In some cases, we need to determine the role of a different user id (e.g. admin endpoint to add a user as organisation admin).

In some cases I separated out the logic from the dependency (used via Depends) and the logic (which can be called separately, outside of Depends).