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

hide account deactivation in OIDC #7652

wants to merge 2 commits into from

Conversation

Velin92
Copy link
Member

@Velin92 Velin92 commented Aug 23, 2023

fixes #7648

This PR also includes a way to make the app remember that the last SSO login was using OIDC. This will only work from the next new hard login.

  • Require the SDK develop branch to be updated.

@Velin92 Velin92 requested a review from pixlwave August 23, 2023 17:42
@sonarcloud
Copy link

sonarcloud bot commented Aug 23, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

warning The version of Java (11.0.14) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Patch coverage: 80.00% and no project coverage change.

Comparison is base (b7ffeda) 12.37% compared to head (049eb8c) 12.37%.
Report is 5 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #7652   +/-   ##
========================================
  Coverage    12.37%   12.37%           
========================================
  Files         1648     1648           
  Lines       163614   163632   +18     
  Branches     67171    67179    +8     
========================================
+ Hits         20242    20251    +9     
- Misses      142707   142716    +9     
  Partials       665      665           
Flag Coverage Δ
uitests 55.04% <100.00%> (-0.01%) ⬇️
unittests 6.22% <80.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
Riot/Modules/Application/LegacyAppDelegate.m 15.60% <0.00%> (-0.01%) ⬇️
...les/Authentication/AuthenticationCoordinator.swift 0.00% <0.00%> (ø)
Riot/Modules/Settings/SettingsViewController.m 0.00% <0.00%> (ø)
.../Legacy/SocialLogin/SocialLoginButtonFactory.swift 80.17% <66.66%> (ø)
...ation/Legacy/SocialLogin/SocialLoginListView.swift 77.77% <66.66%> (ø)
Riot/Managers/Settings/RiotSettings.swift 88.61% <100.00%> (+0.09%) ⬆️
...thentication/Legacy/AuthenticationViewController.m 35.74% <100.00%> (ø)
...s/Authentication/Common/AuthenticationModels.swift 52.38% <100.00%> (+1.16%) ⬆️
...mmon/Service/MatrixSDK/AuthenticationService.swift 71.14% <100.00%> (+1.51%) ⬆️
...ication/Common/Service/MatrixSDK/LoginModels.swift 65.90% <100.00%> (+0.69%) ⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

Nice looks good to me, a few minor comments inline.

@@ -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 😆

@@ -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.

@@ -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


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).


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.

@Velin92 Velin92 closed this Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The app should hide account deactivation in OIDC-aware mode
2 participants