Skip to content

new Settings API#694

Merged
korelstar merged 4 commits into
masterfrom
settings-api
Apr 2, 2021
Merged

new Settings API#694
korelstar merged 4 commits into
masterfrom
settings-api

Conversation

@korelstar

@korelstar korelstar commented Mar 12, 2021

Copy link
Copy Markdown
Member

Provides access to the app settings through the API. This was requested in nextcloud/notes-android#916 (comment). However, my implementation differs to the proposed API. Please check, if this is fine for you, too @stefan-niedermann . My approach was easier 😁
Please note, that the API does not provide the list of allowed file suffixes. This is static for now. We could add a dynamic list later. Is this fine?

  • new API endpoints for getting and setting app settings
  • documentation for API changes
  • Test cases

@korelstar korelstar added the feature: API Related to the API for 3rd-party apps, i.e. Android or iOS label Mar 12, 2021
@korelstar korelstar added this to the 4.1.0 milestone Mar 12, 2021
@stefan-niedermann

Copy link
Copy Markdown
Member

Now that looks awesome, i like the approach 🙂

I am missing two things here in the documentation:

  • Error codes, for example:
    • providing a invalid value as file extension
    • use ../ or / or 1337 as value for the folder
  • Some kind of "reset" - A client does not know the localized term of "Notes" in the server app and has to guess therefore if the user wants to reset the settings to default

@korelstar korelstar force-pushed the settings-api branch 2 times, most recently from 44c0402 to e0d47aa Compare March 13, 2021 09:41
@korelstar

Copy link
Copy Markdown
Member Author

Thanks for the feedback!

Currently, the request succeeds even if the values are invalid. The server performs a sanitation of the values and sends back the fixed values.

  • fileSuffix: if the value is not allowed, the default value is used.
  • notesPath: the path is sanitized, i.e. unnecessary ./, X/../, // are removed.

I've added the following sentences to the documentation:

Empty values are replaced by the settings attribute's default value. All values are sanitized (e.g. prevent path traversal attacks, check allowed suffixes), so the result can differ from the request (the request will still succeed). The client may show an information to the user if the response differs from the request.

Is this sufficient?

@stefan-niedermann

This comment has been minimized.

@korelstar

This comment has been minimized.

@stefan-niedermann

Copy link
Copy Markdown
Member

I have kind of a working implementation, i think. Looking forward to test this with a real server :)

@korelstar

Copy link
Copy Markdown
Member Author

I have kind of a working implementation, i think. Looking forward to test this with a real server :)

What do you mean with "real server"? You can checkout this branch and install it. Did you do such tests in the meanwhile?

@stefan-niedermann

Copy link
Copy Markdown
Member

Nah, nevermind 😄 I just don't have a test instance right now where i could push the state from the branch. When i have time i will set up one.

@korelstar korelstar force-pushed the settings-api branch 2 times, most recently from 8458f57 to b4430e6 Compare April 2, 2021 10:41
@korelstar korelstar marked this pull request as ready for review April 2, 2021 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: API Related to the API for 3rd-party apps, i.e. Android or iOS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants