-
Notifications
You must be signed in to change notification settings - Fork 231
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
Added support for root attributes when creating a new user #287
Conversation
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 about the method signUp
on the Authentication classes, shouldn't it also expose this functionality?
@@ -92,6 +92,11 @@ struct Auth0Authentication: Authentication { | |||
] | |||
payload["username"] = username | |||
payload["user_metadata"] = userMetadata | |||
if let rootAttributes = rootAttributes { | |||
rootAttributes.forEach { (key, value) in | |||
if payload[key] == nil { payload[key] = value } |
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.
this behavior should be clarified in the method documentation
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.
That seems fair, it was intended to stop abuse like overwriting of core params
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.
That's no abuse IMHO. The user should be free to send whatever they want as "additional params". If they send something we already specify, that's their fault.
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 agree with you in principal.
|
||
- returns: request that will yield a created database user (just email, username and email verified flag) | ||
*/ | ||
func createUser(email: String, username: String?, password: String, connection: String, userMetadata: [String: Any]?) -> Request<DatabaseUser, AuthenticationError> | ||
// swiftlint:disable:next function_parameter_count | ||
func createUser(email: String, username: String?, password: String, connection: String, userMetadata: [String: Any]?, rootAttributes: [String: Any]?) -> Request<DatabaseUser, AuthenticationError> |
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.
protocols' methods can be changed without breaking users? this is a public one.
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.
This looks like an optional argument, and it's placed at the end of the parameter list so I'm not sure it would be a breaking change.
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.
The argument is optional so in use you don't need to specify it. However, Lucho is correct any class that implements the protocol still has to support the behaviour. As the Protocol is Public then we should try to avoid breaking it.
I'll add support for this in the Public Extension so the original method will still work out of the box as the method in the Public Extension is a fallback.
It's a deprecated endpoint (non-oidc) |
cbb4904
to
2d26084
Compare
* Added support for root attributes when creating a new user Added Tests * Review Tweaks
Added support for root attributes when creating a new user
Added Tests
Also fixed a
warning
in state string generationChanges
Public API Change to
createUser
which now accepts an optionalrootAttributes
parameter.References
https://auth0.com/docs/api/authentication#signup
Testing
Manually tested as well.
Checklist