Support multiple SSO Identity Providers (MSC2858) #965
Conversation
…r as described in MSC2858.
…ported by the home server.
CHANGES.rst
Outdated
|
|
||
| 🐛 Bugfix | ||
| * | ||
|
|
||
| ⚠️ API Changes | ||
| * | ||
| * Add MXLoginSSOFlow which represents a SSO login flow and offers multiple SSO Identity Providers through MXLoginSSOIdentityProvider class. |
There was a problem hiding this comment.
We usually use the API changes section for api breaks.
It will be annoying if we start to describe every new api here.
| MXJSONModelSetString(loginFlow.type, JSONDictionary[@"type"]); | ||
| MXJSONModelSetArray(loginFlow.stages, JSONDictionary[@"stages"]); |
There was a problem hiding this comment.
Can't we delegate this parsing to the superclass? I mean with another method like parseJSON: in MXLoginfFlow. My concern is that If another property added to MXLoginFlow, we'll have to maintain this one also.
There was a problem hiding this comment.
Yes he have an architecture problem. Here the class method is not really convenient. Member method like other libs would help to make calls to super implementation.
In MXLoginSSOFlow Now we can do something like:
+ (instancetype)modelFromJSON:(NSDictionary *)JSONDictionary
{
MXLoginSSOFlow *loginFlow = [super modelFromJSON:JSONDictionary];
And in MXLoginFlow we should have:
+ (instancetype)modelFromJSON:(NSDictionary *)JSONDictionary
{
MXLoginSSOFlow *loginFlow = [self new];
There was a problem hiding this comment.
I think this seems better.
|
|
||
| MXLoginSSOIdentityProvider *identityProvider = [MXLoginSSOIdentityProvider new]; | ||
|
|
||
| if (identityProvider && identifier && name) |
There was a problem hiding this comment.
Have two concerns.
- We're declaring that
identifierandnamewill be non-nil withNS_ASSUME_NONNULL_BEGIN, but if they are nil after parsing, we still return them nil here. - This introduces some logic in a model class, which doesn't seem to be a good idea.
There was a problem hiding this comment.
Yes I should return nil in this case. In the MSC identifier && name have to be non optional.
MatrixSDK/JSONModels/MXJSONModels.m
Outdated
|
|
||
| NSString *type; | ||
|
|
||
| MXJSONModelSetString(type, JSONDictionary[@"type"]); |
There was a problem hiding this comment.
I'm not very comfortable with exposing the name of type field outside of MXLoginFlow class. Maybe we can define an extern variable for that.
There was a problem hiding this comment.
This should be avoided after #965 (comment)
| /** | ||
| `MXLoginSSOFlow` represents a SSO login or a register flow supported by the home server (See MSC2858 https://github.com/matrix-org/matrix-doc/pull/2858). | ||
| */ | ||
| @interface MXLoginSSOFlow : MXLoginFlow |
There was a problem hiding this comment.
I'm not very comfortable with the existence of this class and deciding that a flow is SSO or not by class check. Why don't we reuse MXLoginFlow for that? And maybe a method like -(BOOL) isSSo, which checks the type.
There was a problem hiding this comment.
I made the same as Web implementation only expose relevant properties for dedicated flow. It's not a really good idea either to expose a lot of properties that can be nil or not and used in only one case.
In MXLoginSSOFlow case we expose the identity providers list can't be nil. And you know that is useful only in this case.
There was a problem hiding this comment.
There was a problem hiding this comment.
Ok, let's continue as is then. thanks for the explanation.
|
|
||
| if let ssoFlow = flows.first(where: { $0.type == kMXLoginFlowTypeSSO }) { | ||
|
|
||
| if let loginSSOFlow = ssoFlow as? MXLoginSSOFlow { |
There was a problem hiding this comment.
This is what i wanted to avoid on above comment.
There was a problem hiding this comment.
He have to differentiate m.login.sso from m.login.cas they are both the same kind of flow but not exactly the same :)
There was a problem hiding this comment.
This will continue as the same, explained in #965 (comment)
Implement MSC2858
Part of element-hq/element-ios/issues/3846