feat: logs for remote access - #26
Conversation
Feat/dd metrics
| await recordDoorUnlock(req.ctx.db, { | ||
| doorId: req.params.doorId, | ||
| doorName: doorDoc?.name, | ||
| username: req.ctx.username ?? req.ctx.userId, | ||
| name: req.ctx.name, | ||
| }); |
There was a problem hiding this comment.
Why are we only recording when a door is unlocked when the user is authed with SSO and not any other possible method?
There was a problem hiding this comment.
Ahh okay it seems like recordDoorUnlock is really only intended for the web. But I guess the question still stands, is there a reason we don't want to record all unlocks in the audit logs?
There was a problem hiding this comment.
Physical door taps are logged separately in server.js
There was a problem hiding this comment.
No, it is inserted into the collection. It still has a console.log which I was using for debugging. That can be cleaned
There was a problem hiding this comment.
They get logged in the db, if you change the date range locally or on gatekeeper.csh to be when doors were online and people were on floor you can see them
There was a problem hiding this comment.
I think I understand this a little better now, but I'm still confused. It seems like there are only two auth methods: secret and oidc. Why would we not want to log for both (and other methods if we ever add them)?
There was a problem hiding this comment.
secret is meant for non-oidc api calls using secrets like GK_DRINK_SECRETS for Drink. It shouldn't be used for unlocking doors because there is no identity attached to it. Good catch W.
What we can do instead is check for userId rather than checking for oidc and deny secret from being used and it also leaves room to add more auth methods later. Physical access is handled via MQTT so there won't be any issue there. Thoughts?
| await recordDoorUnlock(req.ctx.db, { | ||
| doorId: req.params.doorId, | ||
| doorName: doorDoc?.name, | ||
| username: req.ctx.username ?? req.ctx.userId, | ||
| name: req.ctx.name, | ||
| }); |
There was a problem hiding this comment.
Ahh okay it seems like recordDoorUnlock is really only intended for the web. But I guess the question still stands, is there a reason we don't want to record all unlocks in the audit logs?
| username, | ||
| name: name, | ||
| doorsId: null, | ||
| keyId: "Remote Access", |
There was a problem hiding this comment.
This seems like the wrong way of indicating this was a remote unlock. Could this maybe be a separate field?
There was a problem hiding this comment.
Ahh okay it seems like
recordDoorUnlockis really only intended for the web. But I guess the question still stands, is there a reason
we don't want to record all unlocks in the audit logs?
Physical door access is logged in the mqtt handler in server.js, and both physical and remote access land in the same collection. audit logs stores rtp actions for viewing/updating sutff.
This seems like the wrong way of indicating this was a remote unlock. Could this maybe be a separate field?
Yeah, fair, I can add a separate field for that
There was a problem hiding this comment.
Where does the physical access get logged? I can't seem to find it.
There was a problem hiding this comment.
Physical access is logged in the accessLogs collection. If you want to find it on the website you'll have to go back to April or early May. The website shows the past 7 days by default.
edit: I should document stuff on the wiki for future purposes
There was a problem hiding this comment.
Also instead of adding a new field I just made keyId: null for remote access and edited the case for that in the frontend. It is understandable that an entry is remote access if doorsId, keyId, and uid are all null if you are looking the database.
There was a problem hiding this comment.
This still seems pretty unintuitive... Why not just add a type field if its important to distinguish between remote and physical accesses?
There was a problem hiding this comment.
Fair, and I don't disagree that it's less intuitive. I already handle this in the frontend but having it in the database would help too.
I can also add a type field in the keys collection to differentiate NFC card/fob from mobile key. That can be another PR.
…dDoorUnlock, and added a function specifically for checking if door is offline
| function getDoorStatus(doorId){ | ||
| // If it's been more than 1 minute, we assume something is broken... | ||
| const lastHeartbeat = doorHeartbeats.get(req.params.doorId); | ||
| const lastHeartbeat = doorHeartbeats.get(doorId); | ||
| if (lastHeartbeat) { | ||
| res.json({ | ||
| return { | ||
| guess: Date.now() - lastHeartbeat > 1000 * 60 ? "offline" : "online", | ||
| lastHeartbeat, | ||
| }); | ||
| }; | ||
| } else { | ||
| res.json({ | ||
| return { | ||
| guess: "offline", | ||
| lastHeartbeat: 0, | ||
| }); | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Could this use isDoorOffline too so that all that logic is in one spot?
3 participants
What
title
Why
needs logs to keep eye on rtp powers
Test Plan
comment out:
in routes/doors.js
It is somewhat similar stuff to physical door acess logs
Discard the stuff below. I made a stacked PR that'll make testing easy cus doors are offline
In frontend. ahhh create a new branch without the head commitALSO why does vscode keep fucking up whitespaces OMG
Env Vars
N/A
Documentation
N/A
Checklist