chiefonboarding / ChiefOnboarding

Free and open-source employee onboarding platform. Onboard new hires through Slack or the web portal.
https://chiefonboarding.com
GNU Affero General Public License v3.0
656 stars 121 forks source link

Access logging and revoking access #381

Closed GDay closed 10 months ago

GDay commented 10 months ago

Fixes: https://github.com/chiefonboarding/ChiefOnboarding/issues/315

Example (revoke block):

{
    "form": [
        {
            "id": "TEAM_ID",
            "url": "https://app.asana.com/api/1.0/organizations/{{ORG}}/teams",
            "name": "Select team to add user to",
            "type": "choice",
            "method": "GET",
            "data_from": "data",
            "choice_name": "name",
            "choice_value": "gid"
        }
    ],
    "exists": {
        "url": "https://app.asana.com/api/1.0/users/{{email}}",
        "method": "GET",
        "expected": "\"{{email}}\"",
        "fail_when_4xx_response_code": false
    },
    "revoke": [
        {
            "url": "https://app.asana.com/api/1.0/workspaces/{{ORG}}/removeUser",
            "data": {
                "data": {
                    "user": "{{email}}"
                }
            },
            "method": "POST"
        }
    ],
    "execute": [
        {
            "url": "https://app.asana.com/api/1.0/workspaces/{{ORG}}/adUser",
            "data": {
                "data": {
                    "user": "{{email}}"
                }
            },
            "method": "POST"
        },
        {
            "url": "https://app.asana.com/api/1.0/teams/{{TEAM_ID}}/addUser",
            "data": {
                "data": {
                    "user": "{{email}}"
                }
            },
            "method": "POST"
        }
    ],
    "headers": {
        "Accept": "application/json",
        "Content-Type": "application/json",
        "Authorization": "Bearer {{TOKEN}}"
    },
    "initial_data_form": [
        {
            "id": "TOKEN",
            "name": "Please put your token here",
            "description": "You can find your token here: https://...."
        },
        {
            "id": "ORG",
            "name": "Organization id",
            "description": "You can find your organization id here: https://..."
        }
    ]
}
coveralls commented 10 months ago

Coverage Status

coverage: 93.66% (+0.09%) from 93.566% when pulling d73091bd63820e9415b6231155af5379df8c4339 on access-logging-and-revoking-access into e9ee36f0e561b3caae63f517df164c1b7c848f9c on master.

cscheng commented 10 months ago

My initial thought was that revoking users could already be accomplished using existing functionality, for instance by creating custom integrations "Create 3rd party service account" and "Remove 3rd party service account" (and simply log when a certain integration ran for which user). However, having the revoke property in the manifest makes the integration feel more "whole" and it makes sense to have the external API create/delete methods in the same integration/manifest instead of having separate ones.

I would suggest to keep the revoke value similar to execute. The verify option seems a bit redundant, as you expect when the API call is successful the intended action has happened. However, you do need to handle the case when the 3rd party account has been deleted manually before you run this integration. Perhaps this was the intent of verify? Customizing the success message also seems a bit gratuitous, you could create a generic message based on the name of the integration.

In absence of an offboarding sequence, using empty custom integrations to notify an admin to manually remove/disable an account feels like a hack. I understand the need to log when a 3rd party account has been created or deleted, but the validity of the data can be questioned as it has be to input manually. Would an admin task not suffice? Does giving a laptop to a new hire require the same logging? If offboarding sequences are off the table in the foreseeable future, could there be a more elegant solution than empty custom integrations?

I'm also interested in how these logs are made available to the admins to view/use. Does it need to be exported for audit purposes?

GDay commented 10 months ago

Thanks for your thoughts. Definitely some good points.

I would suggest to keep the revoke value similar to execute. The verify option seems a bit redundant, as you expect when the API call is successful the intended action has happened. However, you do need to handle the case when the 3rd party account has been deleted manually before you run this integration. Perhaps this was the intent of verify? Customizing the success message also seems a bit gratuitous, you could create a generic message based on the name of the integration.

When an admin goes to the new hire -> access part, it will fetch the access status live for each integration. Meaning, if someone revoked access through a different way, then it will not show as active anymore.

The verify option seems a bit redundant, as you expect when the API call is successful the intended action has happened.

I added verify for the edge cases in which maybe an API has changed and suddenly needs another call for an account to be fully removed. Though, I think this is rare enough that we can ignore it for now. Worst case, I can just make it mandatory to check before completing the revoke action. Good call.

In absence of an offboarding sequence

Yeah, I should have provided a bit more context. Offboarding sequences are on the list and will be done soon!

Does giving a laptop to a new hire require the same logging?

Hardware tracking is on the list too!

I'm also interested in how these logs are made available to the admins to view/use. Does it need to be exported for audit purposes?

Yeah, again, I should have provided more context. One of the features that will be created is the ability to use the integrations data in the integrations (just like we can use {{email}} etc) to show what integrations have been disabled for the new hire. This info is needed for one API endpoint. For example: create a document with what a user had access to and what has been revoked. So we indeed need to track the "dummy" ones. These will also be added to sequences to be created and also be removed (offboarding).