-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: microsoft graph #357
Conversation
change from bceid to ms graph
log body if unexpected format
return same value on error as before
} else if (status === 'Failed' && failureCode === 'NoResults') { | ||
const url = `${MS_GRAPH_URL}/v1.0/users?$filter=${property} eq '${matchKey}'&$count=true`; | ||
const result = await axios.get(url, options); | ||
if (result && result.data?.value?.length === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the MSGRAPHURL get have a 'not found' response or are we banking on empty response means not found? That can have some unintended consequense if we use this to see if we need to delete a user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left my last comment in a review accidently. There are two checks against that though, axios throws an error on 400/500 status codes, so it will go into the catch block and rethrow. The remove stale users only goes through if it gets the "notexists" string so it wouldn't in those cases.
Then microsoft graph has some docs on its error response here (I tested this as well) which is always a json blob with just an error key. The code logic will only give notextists if there is a defined value key that is empty, which is the case if no users meet criteria. There isn't like a 404 code on that though unfortunately.
It would be better if there was a positive flow instead of a negative flow, e.g IDIM had something like "users deleted in the last 24 hours" endpoint and we only removed those. Since it was built this way though I don't think that exists.
switch form bceid webservice to ms graph for the idir user removals