Skip to content
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

Remove dedicated Microsoft integration user #76

Merged
merged 5 commits into from
Apr 21, 2020
Merged

Conversation

mickmister
Copy link
Contributor

Summary

This PR removes the bot account from the OAuth process. The application level permissions work the exact same way as before.

Calls to GetSchedule are now adjusted to one user's schedule, to have access to the user's event details.

Ticket Link

Fixes #75

@mickmister mickmister added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Apr 8, 2020
@mickmister mickmister requested review from levb and larkox April 8, 2020 01:31
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one comment and one question.

server/mscalendar/availability_test.go Show resolved Hide resolved
server/mscalendar/filters.go Show resolved Hide resolved
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@levb levb removed the 2: Dev Review Requires review by a core committer label Apr 8, 2020
@levb levb requested a review from DHaussermann April 8, 2020 22:05
@levb levb added this to the 0.1.0 milestone Apr 11, 2020
@DHaussermann
Copy link

@mickmister I started over with a fresh server and tested an install without a bot.
Seems like everything works fine except 2 command...

/mscalendar viewcal
/mscalendar summary view

They produce the following error: msg":"Unable to execute command.","path":"/api/v4/commands/execute","request_id":"zzex7xgc8p8qdydunaunguto3e","ip_addr":"...","user_id":"g1a5x4iu9f883gmbwgemdxh6ch","method":"POST","err_where":"mscalendarplugin.ExecuteCommand","http_code":500,"err_details":"Command /mscalendar summary failed: 403 Forbidden: {\"error\":{\"code\":\"ErrorAccessDenied\",\"message\":\"Access is denied. Check credentials and try again.\"}}"}

All other slash commands seem to work fine.

If this is related to a bot-less install, possibly there is an update needed to how permissions are configured. Thoughts?

Also Please remove Step 3: Configure Bot Account from the read-me.

@mickmister
Copy link
Contributor Author

@DHaussermann I'm unable to reproduce the issues with /mscalendar viewcal and
/mscalendar summary view on a fresh install. I've removed the bot account instructions from the README.

@codecov-io
Copy link

Codecov Report

Merging #76 into master will decrease coverage by 2.71%.
The diff coverage is 50.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
- Coverage   25.31%   22.59%   -2.72%     
==========================================
  Files          61       61              
  Lines        2303     2248      -55     
==========================================
- Hits          583      508      -75     
- Misses       1654     1682      +28     
+ Partials       66       58       -8     
Impacted Files Coverage Δ
server/command/command.go 29.41% <ø> (-3.93%) ⬇️
server/command/connect.go 100.00% <ø> (ø)
server/command/disconnect.go 100.00% <ø> (ø)
server/mscalendar/user.go 5.26% <ø> (-14.33%) ⬇️
server/remote/msgraph/remote.go 4.54% <0.00%> (-1.34%) ⬇️
server/mscalendar/filters.go 28.94% <12.50%> (-13.56%) ⬇️
server/remote/msgraph/get_schedule_batched.go 40.00% <59.25%> (-11.67%) ⬇️
server/mscalendar/availability.go 46.36% <100.00%> (+0.34%) ⬆️
server/mscalendar/client.go 28.57% <100.00%> (-31.43%) ⬇️
server/mscalendar/oauth2.go 86.79% <100.00%> (-2.44%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4cd92a...e6174ce. Read the comment docs.

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and passed.

  • Tested install without bot service user
  • Ran through brief regression testing
  • Issues found above were due to mis-configured Azure app permissions (Future doc change coming to clarify this) The issue where unrelated to bot removal.
  • No other issues found
    LGTM! Thanks @mickmister

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request QA Review Done PR has been approved by QA and removed 3: QA Review Requires review by a QA tester labels Apr 21, 2020
@mickmister mickmister merged commit c134e84 into master Apr 21, 2020
@mickmister mickmister deleted the remove-bot-account branch April 21, 2020 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request QA Review Done PR has been approved by QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove dedicated bot Microsoft user
5 participants