Skip to content

Conversation

AngeloStavrow
Copy link
Collaborator

Closes #105.

This PR includes some cleanup. It turns off smart-dashes for the iOS body-text editor (to match the Mac app's behaviour per #168), and refactors the WriteFreely model into four files:

  • Shared/Models/WriteFreelyModel → The WriteFreely model object class
  • Shared/Extensions/WriteFreelyModel+API.swift → The WriteFreely model's API
  • Shared/Extensions/WriteFreelyModel+APIHandlers.swift → Handlers associated with the WriteFreely model's API
  • Shared/Extensions/WriteFreelyModel+Keychain.swift → Helper methods for working with the system Keychain

@AngeloStavrow AngeloStavrow added ios Anything specific to the iPhone/iPad app mac Anything specific to the Mac app labels Feb 1, 2021
@AngeloStavrow AngeloStavrow added this to the 1.0 milestone Feb 1, 2021
@AngeloStavrow AngeloStavrow requested a review from thebaer February 1, 2021 19:31
@AngeloStavrow AngeloStavrow self-assigned this Feb 1, 2021
@thebaer
Copy link
Member

thebaer commented Feb 1, 2021

Looking at this, I'm just thinking of different edge cases and wondering if there's any way we can have a more friendly error message when someone gives a bad hostname (e.g. "writefreely" or "write.freely" or "write.as//" or "write.as/matt"). Do you think that'd be pretty involved?

I'm just thinking about how we make this as robust as possible, and handle a wide variety of invalid inputs, rather than just this one particular case.

@AngeloStavrow
Copy link
Collaborator Author

AngeloStavrow commented Feb 1, 2021

Looking at this, I'm just thinking of different edge cases and wondering if there's any way we can have a more friendly error message when someone gives a bad hostname (e.g. "writefreely" or "write.freely" or "write.as//" or "write.as/matt"). Do you think that'd be pretty involved?

We could try extending NSDataDetector to check if a link is valid — there's an example here that seems to be more useful for our use case than, for example, checking against a regex.

However, that won't catch the "write.as/matt" case, because that's a valid URL, so I guess we'd still have to check that individually. I'd probably just validate that similar to the trailing-slash case: if server.hasPrefix("write.as") (or a variant with the protocol), just rewrite server string the value as "https://write.as" and proceed with logging in.

Right now we just rely on getting back an error after sending the login request to the server to trigger the .serverNotFound case in the AccountError enum. I think it's worth keeping that flow, and adding a pre-flight check to the login method that returns a new .serverURLIsInvalid case before bothering with a network request.

If that sounds like a good plan, I'll edit the linked issue's title to be "Make server URL validation more robust" for this work. 👍

@thebaer
Copy link
Member

thebaer commented Feb 1, 2021

Could you just parse the hostname out of the user input, and use that? Here's an example of what I mean, written in Go. I believe Swift provides similar URL parsing abilities?

The critical goal here is that the app helps the user enter in the correct Instance URL (i.e. a valid URL and the correct instance). So I think the ideal UX would be: the app takes the Instance URL, automatically makes it valid, and then only ever complains to the user if they entered in a URL that isn't a WriteFreely instance.

So once we have a valid URL, are we also showing a specific error message that would help them to this end when the .serverNotFound error comes up?

@AngeloStavrow
Copy link
Collaborator Author

AngeloStavrow commented Feb 1, 2021

Perfect, that clarifies for me. We can use URLComponents to get the protocol and host, which will return nil if the URL is invalid (e.g., someone enters writefreely as the server URL).

@AngeloStavrow AngeloStavrow marked this pull request as draft February 1, 2021 22:03
@AngeloStavrow
Copy link
Collaborator Author

We can use URLComponents to get the protocol and host, which will return nil if the URL is invalid (e.g., someone enters writefreely as the server URL).

Turns out that, according to URLComponents, writefreely is a perfectly valid URL, and will therefore never fail. ¯\_(ツ)_/¯

Let me find a workaround, maybe using the NSDataDetector option above.

@thebaer
Copy link
Member

thebaer commented Feb 2, 2021

I guess that makes sense if you have a local server named writefreely (though I don't imagine that'll often be the case for our users). Maybe we consider this input "valid" and don't worry about handling that case here -- and instead rely on other error handling to inform the user that they entered the incorrect server URL?

@AngeloStavrow
Copy link
Collaborator Author

Yeah, that's what I think we have to do. NSDataDetector recognizes the same as a URL, so we can't count on that. Which, as you point out, makes sense.

I imagine that there's some case where a string is not considered a valid URL (like emoji?), so there's error handling that'll catch this and present the following alert:

Otherwise, we just have to let the app make the request and see what happens.

@AngeloStavrow AngeloStavrow marked this pull request as ready for review February 2, 2021 16:13
Copy link
Member

@thebaer thebaer left a comment

Choose a reason for hiding this comment

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

Good to go! Thanks.

@AngeloStavrow AngeloStavrow merged commit 3d5da9c into main Feb 2, 2021
@AngeloStavrow AngeloStavrow deleted the remove-trailing-slash-in-serverURL branch February 2, 2021 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ios Anything specific to the iPhone/iPad app mac Anything specific to the Mac app
Development

Successfully merging this pull request may close these issues.

Make server URL validation more robust
2 participants