-
Notifications
You must be signed in to change notification settings - Fork 108
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
Enterprise Connection only authentication #336
Conversation
Add Enterprise Connection Support Add Enterprise CDN Parsing Add Enterprise Presentor Add Enterprise Interactor Add Enterprise View Add Domain Validator
More protocols usage Enterprise Error Types Improve Domain Validator Action Enterprise Presenter
UI Clean Tidied Logging Added Enterprise View
- parameter domain: primary domain of connection | ||
- parameter domainAlias: array of domaina aliases | ||
*/ | ||
mutating func enterprise(name name: String, strategy: String, domain: String, domainAlias: [String]) |
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.
Let's coalesce into a single domain
parameter of type [String]
. And make sure when we parse the info from the CDN we put the domain
one first.
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.
Makes sense, I was focusing on AD which is just a comma list in dashboard that presents then first domain in the list as the primary domain. Will change.
@@ -59,3 +60,17 @@ public struct SocialConnection: OAuth2Connection { | |||
public let name: String | |||
public let style: AuthStyle | |||
} | |||
|
|||
public protocol EnterpriseConnection { |
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 we don't need this protocol now right? or it has some use I don't see?
|
||
public protocol EnterpriseConnection { | ||
var name: String { get } | ||
var strategy: String { get } |
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.
What's the use of strategy
?
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 thought it might be useful to know the enterprise strategy, if AD present this view etc
} | ||
|
||
func requestConnection(callback: (OAuth2AuthenticatableError?) -> ()) { | ||
guard let _ = self.email else { return callback(.NoConnectionAvailable) } |
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.
probably best to use self.email != nil
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.
It feels less swifty in comparison to guard. However, I also do not like using let _
to suppress warning.
authenticator.login(connection.name) { error in | ||
return callback(error) | ||
} | ||
callback(nil) |
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.
Is this right?, shouldn't it be inside of the callback?. Maybe just login like
authenticator.login(connection.name, callback);
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'll try this it's cleaner.
self.connections = connections | ||
} | ||
|
||
func validate(value: String?) -> ErrorType? { |
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.
Not bad but a bit objc 😄 .
func validate(value: String?) -> ErrorType? {
guard let domain = value?.componentsSeparatedByString("@").last else { return InputValidationError.MustNotBeEmpty }
self.enterpriseConnection = connections.filter { $0.domainAlias.contains(domain) }.first
return self.enterpriseConnection
}
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.
Not sure it works now since I think the domain
is not present in domainAlias
but with my other comment it should be fixed
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.
lol yeah, was a homage to your previous code ;) Will make it swifty.
public class EnterpriseDomainValidator : InputValidator { | ||
|
||
var connections: [EnterpriseConnection] | ||
var enterpriseConnection: EnterpriseConnection? |
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 am not completely convinced of storing this here but since it's a validator I see no way out. Maybe it's better to avoid making this a validator since they should be stateless.
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.
Will review, I dislike doing this also, just no other way I could think to do it as a validator.
Little tidy up on enterprise naming
- parameter name: name of the connection | ||
- parameter domain: array of enterprise domains | ||
*/ | ||
mutating func enterprise(name name: String, domain: [String]) |
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.
Let's use domains
|
||
import Foundation | ||
|
||
protocol EnterpriseDomain { |
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.
Let's rename this to HRDAuthenticatable
Tweaked logging
No description provided.