-
Notifications
You must be signed in to change notification settings - Fork 498
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
Conversation
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information 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. |
Codecov ReportPatch coverage:
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
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this 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]; |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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)} |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
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.