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

hide account deactivation in OIDC #7652

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Riot/Managers/Settings/RiotSettings.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ final class RiotSettings: NSObject {
static let enableUISIAutoReporting = "enableUISIAutoReporting"
static let enableLiveLocationSharing = "enableLiveLocationSharing"
static let showIPAddressesInSessionsManager = "showIPAddressesInSessionsManager"
static let isAuthenticatedWithOIDC = "isAuthenticatedWithOIDC"
}

static let shared = RiotSettings()
Expand Down Expand Up @@ -74,6 +75,9 @@ final class RiotSettings: NSObject {
@UserDefault(key: "identityserverurl", defaultValue: BuildSettings.serverConfigDefaultIdentityServerUrlString, storage: defaults)
var identityServerUrlString

@UserDefault(key: UserDefaultsKeys.isAuthenticatedWithOIDC, defaultValue: false, storage: defaults)
var isAuthenticatedWithOIDC

// MARK: Notifications

/// Indicate if `showDecryptedContentInNotifications` settings has been set once.
Expand Down
2 changes: 2 additions & 0 deletions Riot/Modules/Application/LegacyAppDelegate.m
Original file line number Diff line number Diff line change
Expand Up @@ -2190,6 +2190,8 @@ - (void)logoutSendingRequestServer:(BOOL)sendLogoutServerRequest

[TimelinePollProvider.shared reset];

RiotSettings.shared.isAuthenticatedWithOIDC = NO;

#ifdef MX_CALL_STACK_ENDPOINT
// Erase all created certificates and private keys by MXEndpointCallStack
for (MXKAccount *account in MXKAccountManager.sharedManager.accounts)
Expand Down
2 changes: 2 additions & 0 deletions Riot/Modules/Authentication/AuthenticationCoordinator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -681,6 +681,8 @@ extension AuthenticationCoordinator: SSOAuthenticationPresenterDelegate {
return
}

RiotSettings.shared.isAuthenticatedWithOIDC = identityProvider?.isOIDC ?? false

Task { await handleLoginToken(token, using: loginWizard) }
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1524,7 +1524,7 @@ - (BOOL)isSocialLoginViewShown

- (CGFloat)socialLoginViewHeightFittingWidth:(CGFloat)width
{
NSArray<MXLoginSSOIdentityProvider*> *identityProviders = self.currentLoginSSOFlow.identityProviders;
NSArray<SSOIdentityProvider*> *identityProviders = self.currentLoginSSOFlow.ssoIdentityProviders;

if (!identityProviders.count && self.socialLoginListView)
{
Expand All @@ -1546,7 +1546,7 @@ - (void)showSocialLoginViewWithLoginSSOFlow:(MXLoginSSOFlow*)loginSSOFlow andMod
listView.delegate = self;
}

[listView updateWith:loginSSOFlow.identityProviders mode:mode];
[listView updateWith: loginSSOFlow.ssoIdentityProviders mode:mode];
Copy link
Member

Choose a reason for hiding this comment

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

Too much time writing Swift, I don't think you need the new in there 😆


[self refreshContentViewHeightConstraint];
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class SocialLoginButtonFactory {

// MARK - Public

func build(with identityProvider: MXLoginSSOIdentityProvider, mode: SocialLoginButtonMode) -> SocialLoginButton {
func build(with identityProvider: SSOIdentityProvider, mode: SocialLoginButtonMode) -> SocialLoginButton {
let button = SocialLoginButton()

let defaultStyle: SocialLoginButtonStyle
Expand All @@ -37,7 +37,7 @@ class SocialLoginButtonFactory {
let buildDefaultButtonStyles: () -> (SocialLoginButtonStyle, [String: SocialLoginButtonStyle]) = {
let image: SourceImage?

if let imageStringURL = identityProvider.icon, let imageURL = URL(string: imageStringURL) {
if let imageStringURL = identityProvider.iconURL, let imageURL = URL(string: imageStringURL) {
image = .remote(imageURL)
} else {
image = nil
Expand Down Expand Up @@ -71,7 +71,7 @@ class SocialLoginButtonFactory {

let title = self.buildButtonTitle(with: identityProvider.name, mode: mode)

let viewData = SocialLoginButtonViewData(identityProvider: identityProvider.ssoIdentityProvider,
let viewData = SocialLoginButtonViewData(identityProvider: identityProvider,
title: title,
defaultStyle: defaultStyle,
themeStyles: styles)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ final class SocialLoginListView: UIView, NibLoadable {

// MARK: - Public

func update(with identityProviders: [MXLoginSSOIdentityProvider], mode: SocialLoginButtonMode) {
func update(with identityProviders: [SSOIdentityProvider], mode: SocialLoginButtonMode) {
self.mode = mode

let title: String
Expand Down Expand Up @@ -89,7 +89,7 @@ final class SocialLoginListView: UIView, NibLoadable {
self.buttons = buttons
}

static func contentViewHeight(identityProviders: [MXLoginSSOIdentityProvider],
static func contentViewHeight(identityProviders: [SSOIdentityProvider],
mode: SocialLoginButtonMode,
fitting width: CGFloat) -> CGFloat {
let sizingView = self.sizingView
Expand All @@ -113,7 +113,7 @@ final class SocialLoginListView: UIView, NibLoadable {
self.buttons = []
}

private func socialLoginButtons(for identityProviders: [MXLoginSSOIdentityProvider], mode: SocialLoginButtonMode) -> [SocialLoginButton] {
private func socialLoginButtons(for identityProviders: [SSOIdentityProvider], mode: SocialLoginButtonMode) -> [SocialLoginButton] {

var buttons: [SocialLoginButton] = []

Expand All @@ -122,7 +122,7 @@ final class SocialLoginListView: UIView, NibLoadable {
if let firstIdentityProviderBrand = firstIdentityProvider.brand, let secondIdentityProviderBrand = secondIdentityProvider.brand {
return firstIdentityProviderBrand < secondIdentityProviderBrand
} else {
return firstIdentityProvider.identifier < secondIdentityProvider.identifier
return firstIdentityProvider.id < secondIdentityProvider.id
}
}

Expand Down
4 changes: 2 additions & 2 deletions Riot/Modules/Settings/SettingsViewController.m
Original file line number Diff line number Diff line change
Expand Up @@ -607,8 +607,8 @@ - (void)updateSections
[tmpSections addObject:sectionLabs];
}
}

if (BuildSettings.settingsScreenAllowDeactivatingAccount)
if (BuildSettings.settingsScreenAllowDeactivatingAccount && !RiotSettings.shared.isAuthenticatedWithOIDC)
{
Section *sectionDeactivate = [Section sectionWithTag:SECTION_TAG_DEACTIVATE_ACCOUNT];
[sectionDeactivate addRowWithTag:0];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,14 @@ class HomeserverAddress: NSObject {
let brand: String?
/// The icon field is an optional field that points to an icon representing the identity provider. If present then it must be an HTTPS URL to an image resource.
let iconURL: String?
/// Dertermines if this provider uses OIDC
let isOIDC: Bool
Copy link
Member

Choose a reason for hiding this comment

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

I think for sanity this should also be delegatedOIDCCompatibility.


init(id: String, name: String, brand: String?, iconURL: String?) {
init(id: String = "", name: String, brand: String?, iconURL: String?, isOIDC: Bool = false) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really a fan of the default value for id now I look at it. It feels optional now, whereas it really isn't. Would be better imo to explicitly set this when we create the mock provider with a comment that its fine as there is only one.

self.id = id
self.name = name
self.brand = brand
self.iconURL = iconURL
self.isOIDC = isOIDC
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -290,9 +290,16 @@ class AuthenticationService: NSObject {
// Get the login flow
let loginFlowResponse = try await client.getLoginSession()

let identityProviders = loginFlowResponse.flows?.compactMap { $0 as? MXLoginSSOFlow }.first?.identityProviders ?? []
let loginFlow = loginFlowResponse.flows?.compactMap { $0 as? MXLoginSSOFlow }.first
var identityProviders = loginFlow?.ssoIdentityProviders ?? []
if identityProviders.isEmpty {
// Provide a backup for homeservers that support SSO but don't offer any identity providers
// https://spec.matrix.org/latest/client-server-api/#client-login-via-sso
identityProviders = [SSOIdentityProvider(name: "SSO", brand: nil, iconURL: nil, isOIDC: loginFlow?.delegatedOIDCCompatibility ?? false)]
}

return LoginFlowResult(supportedLoginTypes: loginFlowResponse.flows?.compactMap { $0 } ?? [],
ssoIdentityProviders: identityProviders.sorted { $0.name < $1.name }.map(\.ssoIdentityProvider),
ssoIdentityProviders: identityProviders.sorted { $0.name < $1.name },
homeserverAddress: client.homeserver)
}

Expand All @@ -309,7 +316,14 @@ class AuthenticationService: NSObject {
}

extension MXLoginSSOIdentityProvider {
var ssoIdentityProvider: SSOIdentityProvider {
SSOIdentityProvider(id: identifier, name: name, brand: brand, iconURL: icon)
@objc func makeSSOIdentityProvider(isOIDC: Bool) -> SSOIdentityProvider {
Copy link
Member

Choose a reason for hiding this comment

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

Same on this property name

SSOIdentityProvider(id: identifier, name: name, brand: brand, iconURL: icon, isOIDC: isOIDC)
}
}

extension MXLoginSSOFlow {
@objc var ssoIdentityProviders: [SSOIdentityProvider] {
identityProviders.map { $0.makeSSOIdentityProvider(isOIDC: delegatedOIDCCompatibility)}
Copy link
Member

Choose a reason for hiding this comment

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

I would question if this part is correct. AIUI, if there are multiple providers, then the presence of this flag means "don't show them anyway" but as we're not doing that, I would be surprised if these are then using OIDC compatibility (as they're basically Sign in with Google/Apple etc which aren't).

}
}

Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,7 @@ enum LoginMode {
var ssoIdentityProviders: [SSOIdentityProvider]? {
switch self {
case .sso(let ssoIdentityProviders), .ssoAndPassword(let ssoIdentityProviders):
// Provide a backup for homeservers that support SSO but don't offer any identity providers
// https://spec.matrix.org/latest/client-server-api/#client-login-via-sso
return ssoIdentityProviders.count > 0 ? ssoIdentityProviders : [SSOIdentityProvider(id: "", name: "SSO", brand: nil, iconURL: nil)]
return ssoIdentityProviders
default:
return nil
}
Expand Down
1 change: 1 addition & 0 deletions changelog.d/7648.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Deactivate account option is disabled when logging in with OIDC.
Loading