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

open an external browser when the server config for MobileExternalBrowse is set to true #8220

Merged
merged 2 commits into from
Sep 16, 2024
Merged

open an external browser when the server config for MobileExternalBrowse is set to true #8220

merged 2 commits into from
Sep 16, 2024

Conversation

enahum
Copy link
Contributor

@enahum enahum commented Sep 13, 2024 •
edited
Loading

Summary

Introducing a client config property that determines if the mobile app should use the default external browser to perform SSO Authentication instead of the internal one. mattermost/mattermost#28180

Important: We want to include this in a dot release so that ships as soon as possible.

Also addresses an issue reported in #8206

Ticket Link

https://mattermost.atlassian.net/browse/MM-60332
Fixes: #8206
Fixes: #8155

Release Note

Added config setting NativeAppSettings -> MobileExternalBrowser that tells the mobile app to perform the SSO Authentication using the external default browser

@enahum enahum added 2: Dev Review Requires review by a core commiter CherryPick/Candidate A candidate for a quality or patch release, but not yet approved labels Sep 13, 2024
@darkweizer darkweizer mentioned this pull request Sep 13, 2024
Closed
@amyblais amyblais removed their request for review September 13, 2024 12:08
larkox
larkox approved these changes Sep 13, 2024
View reviewed changes
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, but one nit and one thing to consider.

app/screens/sso/components/auth_error.tsx Show resolved Hide resolved
app/screens/sso/sso_authentication_with_external_browser.tsx Show resolved Hide resolved
rahimrahman
rahimrahman approved these changes Sep 13, 2024
View reviewed changes
Copy link
Contributor

@rahimrahman rahimrahman left a comment •
edited
Loading

Choose a reason for hiding this comment

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

👍🏽

I just have 1 non-blocking comment in regards to ease of testing this.

@amyblais amyblais removed the 2: Dev Review Requires review by a core commiter label Sep 13, 2024
rahimrahman
rahimrahman reviewed Sep 13, 2024
View reviewed changes
app/screens/sso/index.tsx
@@ -155,14 +156,28 @@ const SSO = ({
theme,
};

let authentication;
if (config.MobileExternalBrowser === 'true') {
Copy link
Contributor

@rahimrahman rahimrahman Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@enahum any chance we can or should make it easier to pass an env so that we can do any type of test without having to update the server?

I was thinking about E2E test, or for QA to test without having to modify the config if we need to test this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it could be done by the time we introduce e2e for this. That e2e will require the server to be configured with a SSO anyway

@amyblais amyblais added the 2: Dev Review Requires review by a core commiter label Sep 13, 2024
@enahum enahum added Build App for Android Build the mobile app for Android 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Sep 13, 2024
@amyblais amyblais added this to the v2.21.0 milestone Sep 16, 2024
@amyblais amyblais added CherryPick/Approved Meant for the quality or patch release tracked in the milestone and removed CherryPick/Candidate A candidate for a quality or patch release, but not yet approved labels Sep 16, 2024
@enahum enahum merged commit f04838d into main Sep 16, 2024
36 checks passed
@mattermost-build
Copy link
Contributor

Cherry pick is scheduled.

@enahum enahum deleted the MM-60332 branch September 16, 2024 22:40
mattermost-build pushed a commit that referenced this pull request Sep 16, 2024
@enahum @mattermost-build
open an external browser when the server config for MobileExternalBro…
564c198 
…wse is set to true (#8220)

(cherry picked from commit f04838d)
@mattermost-build mattermost-build mentioned this pull request Sep 16, 2024
Merged
@mattermost-build mattermost-build added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Sep 16, 2024
amyblais pushed a commit that referenced this pull request Sep 17, 2024
@mattermost-build @enahum
open an external browser when the server config for MobileExternalBro…
d91023d 
…wse is set to true (#8220) (#8223)

(cherry picked from commit f04838d)

Co-authored-by: Elias Nahum <nahumhbl@gmail.com>
@amyblais amyblais added the Docs/Needed Requires documentation label Sep 17, 2024
@amyblais
Copy link
Member

amyblais commented Oct 8, 2024

/cherry-pick release-2.20

@mattermost-build
Copy link
Contributor

Cherry pick is scheduled.

mattermost-build pushed a commit that referenced this pull request Oct 8, 2024
@enahum @mattermost-build
open an external browser when the server config for MobileExternalBro…
ee2fd59 
…wse is set to true (#8220)

(cherry picked from commit f04838d)
@mattermost-build mattermost-build mentioned this pull request Oct 8, 2024
Merged
amyblais pushed a commit that referenced this pull request Oct 8, 2024
@mattermost-build @enahum
open an external browser when the server config for MobileExternalBro…
2a73d64 
…wse is set to true (#8220) (#8247)

(cherry picked from commit f04838d)

Co-authored-by: Elias Nahum <nahumhbl@gmail.com>
@cwarnermm cwarnermm mentioned this pull request Oct 8, 2024
Closed
larkox pushed a commit that referenced this pull request Oct 11, 2024
@enahum @larkox
open an external browser when the server config for MobileExternalBro…
03f1436 
…wse is set to true (#8220)
@cwarnermm cwarnermm added Docs/Done Required documentation has been written and removed Docs/Needed Requires documentation labels Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rahimrahman rahimrahman rahimrahman approved these changes

@larkox larkox larkox approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Build App for Android Build the mobile app for Android CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone Docs/Done Required documentation has been written release-note
Projects
None yet
Milestone
v2.21.0
Development

Successfully merging this pull request may close these issues.

SSO page connection not open browser page login with otp on same device fails
7 participants
@enahum @mattermost-build @amyblais @larkox @rahimrahman @mm-cloud-bot @cwarnermm