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.

Sign up for GitHub

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

Jump to bottom

Azure single-tenant application support, using the Graph API #6728

Merged
merged 11 commits into from
Apr 11, 2023
Merged

Azure single-tenant application support, using the Graph API #6728

merged 11 commits into from
Apr 11, 2023

Conversation

moxvallix
Copy link

@moxvallix moxvallix commented Mar 22, 2023 •
edited
Loading

Description

This pull request adds support to Chatwoot for using single tenant Azure applications.

When the AZURE_TENANT_ID environment variable is set, Chatwoot will now use it within it's calls to Microsoft's API, rather than common, which only works for multi tenant applications.

As well, this pull request also adds support for sending and retrieving emails through the Microsoft Graph API, rather than using IMAP / SMTP. This was because I was unable to get SMTP working with a single tenant application. As well, using this API will likely be needed in the future, as Microsoft are deprecating SMTP Auth.

https://learn.microsoft.com/en-us/exchange/clients-and-mobile-in-exchange-online/deprecation-of-basic-authentication-exchange-online#pop-imap-and-smtp-auth

I would appreciate some help with getting this pull request up to standards. As it is currently not, I will leave it as a draft for now.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

I have been hand testing this all myself in my local environment, but some help/guidance with writing proper tests would be appreciated.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented on my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@pr-triage pr-triage bot added the PR: draft label Mar 22, 2023
@netlify
Copy link

netlify bot commented Mar 22, 2023 •
edited
Loading

✅ Deploy Preview for chatwoot-storybook ready!

Name Link
🔨 Latest commit 479166f
🔍 Latest deploy log https://app.netlify.com/sites/chatwoot-storybook/deploys/6423ce29c969e2000784a71b
😎 Deploy Preview https://deploy-preview-6728--chatwoot-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@evolve2k evolve2k mentioned this pull request Mar 22, 2023
Closed
@sojan-official sojan-official added the community PRs from the community label Mar 23, 2023
@tejaswinichile
Copy link
Contributor

Will take a look 👍🏼

@tejaswinichile
Copy link
Contributor

@moxvallix Would this change support the single tenant changes as well?

@moxvallix
Copy link
Author

@tejaswinichile sorry, I am not sure what other single tenant changes you refer to. Has there been changes to support single tenant applications in chatwoot since I made this branch?

@tejaswinichile
Copy link
Contributor

tejaswinichile commented Mar 28, 2023 •
edited
Loading

sorry, I am not sure what other single tenant changes you refer to. Has there been changes to support single tenant applications in chatwoot since I made this branch?

@moxvallix Concerned about the users who chose the single tenant as an account type, do you think this would work for that? And they will still need to update the tennt id?

Screenshot 2023-03-28 at 12 17 56 PM

@moxvallix
Copy link
Author

@tejaswinichile yes, these changes are to make single tenant applications work. The AZURE_TENANT_ID is required, as single tenant applications must authenticate to their own endpoint, which uses this id, rather than the common endpoint, which multi tenant use.

This PR should have made no changes to how multi tenant works, but should have single tenant working.

@tejaswinichile
Copy link
Contributor

tejaswinichile commented Mar 29, 2023 •
edited
Loading

The PR overall seems alright; I will work on it further as I want to test if the customer has setup an email channel before the new endpoint, and the fetch process should be seamless with the update of tenant id.

I will make some changes for the review and will add the test cases as well.

cc @sojan-official

@moxvallix
Copy link
Author

moxvallix commented Mar 30, 2023 •
edited
Loading

It won't work with pre-existing, single tenant app channels. It requires a seperate OAuth token, to grant scopes for the Graph API.

Considering that before this it was impossible to add channels from single tenant apps anyways, this shouldn't be an issue.

If used with a multi tenant app, not setting AZURE_TENANT_ID will keep everything functioning as before.

tejaswinichile
tejaswinichile suggested changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@tejaswinichile tejaswinichile left a comment

Choose a reason for hiding this comment

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

@moxvallix Let me know if you get time to fix these, or I will.

app/controllers/concerns/microsoft_concern.rb Show resolved Hide resolved
app/jobs/inboxes/fetch_imap_email_inboxes_job.rb Show resolved Hide resolved
app/jobs/inboxes/fetch_ms_graph_emails_job.rb Show resolved Hide resolved
@AwakenTheJaken
Copy link

Any status updates on this? We are currently having the same issue mentioned in the above. Appreciate the your guys' work @moxvallix and @tejaswinichile !

@moxvallix
Copy link
Author

@AwakenTheJaken it should all be working, the branch basically just needs conformity changes, tests etc. You should be good to pull down these changes into your own instance (make sure you set AZURE_TENANT_ID to the ID of your Azure tenant).

@AwakenTheJaken
Copy link

@AwakenTheJaken it should all be working, the branch basically just needs conformity changes, tests etc. You should be good to pull down these changes into your own instance (make sure you set AZURE_TENANT_ID to the ID of your Azure tenant).

Thank you, appreciate it!~

@tejaswinichile
Copy link
Contributor

@moxvallix Are you going to work on this es/fetch_imap_email_inboxes_job.rb ?

@moxvallix
Copy link
Author

moxvallix commented Apr 11, 2023 •
edited
Loading

@moxvallix Are you going to work on this es/fetch_imap_email_inboxes_job.rb ?

Not sure exactly what is needed here? FetchImapEmailsJob already handles Microsoft email accounts when the TENANT_ID is not set. Unless you are wanting me to make it that the Graph API is used for multi-tenant apps as well?

@tejaswinichile
Copy link
Contributor

Not sure exactly what is needed here? FetchImapEmailsJob already handles Microsoft email accounts when the TENANT_ID is not set. Unless you are wanting me to make it that the Graph API is used for multi-tenant apps as well?

@moxvallix No. Just confirming if you are done with all the changes, I will open it for review and make the necessary changes from my side.

Otherwise, PR works well.

@tejaswinichile tejaswinichile marked this pull request as ready for review April 11, 2023 05:15
@moxvallix
Copy link
Author

@moxvallix No. Just confirming if you are done with all the changes, I will open it for review and make the necessary changes from my side.

Otherwise, PR works well.

Ah okay, no worries. Yeah, I'm done with changes to this branch, I don't have much time to work on it right now regardless. All good for you to do whatever you need with it.

@tejaswinichile tejaswinichile changed the base branch from develop to azure-test-branch April 11, 2023 06:49
@tejaswinichile tejaswinichile merged commit 54897df into chatwoot:azure-test-branch Apr 11, 2023
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tejaswinichile tejaswinichile tejaswinichile requested changes

Assignees

@tejaswinichile tejaswinichile

Labels
community PRs from the community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@moxvallix @tejaswinichile @AwakenTheJaken @sojan-official