Handle public instances #126

davidoskky · 2023-01-26T17:09:01Z

davidoskky commented

2023-01-26 17:09:01 +00:00

Types of changes

I have read the CONTRIBUTING document.
My code follows the code style of this project.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.
This is NOT translation related.

This is implements feature #106

This supports public instances by disabling any kind of unauthorized action.
There's a problem with sources: these are not correctly being fetched because we use the /sources/list api endpoint which is not publicly accessible rather than /sources/stats which is. https://github.com/fossar/selfoss/issues/1403
I'm unsure whether to include changes to fetching sources in this PR or to create a new one for that. This is currently complete and working, however it's impossible to filter by source.

## Types of changes - [x] I have read the **CONTRIBUTING** document. - [x] My code follows the code style of this project. - [ ] I have updated the documentation accordingly. - [x] I have added tests to cover my changes. - [x] All new and existing tests passed. - [x] This is **NOT** translation related. This is implements feature #106 This supports public instances by disabling any kind of unauthorized action. There's a problem with sources: these are not correctly being fetched because we use the /sources/list api endpoint which is not publicly accessible rather than /sources/stats which is. https://github.com/fossar/selfoss/issues/1403 I'm unsure whether to include changes to fetching sources in this PR or to create a new one for that. This is currently complete and working, however it's impossible to filter by source.

davidoskky added 7 commits 2023-01-26 17:09:02 +00:00

Fetch remote selfoss configuration d0f649bb27

Fetch from /api/about the selfoss configuration to determine if we're using a public access instanceIf both authentication and public mode are enabled in the configuration and we're logging in without authentication, then we're using public access.

Fix broken tests 529ada8882

Add tests for the public access determination logic e3e8cc8a79

Handle public access in the home screen 04f3937095

In public access mode we can only read articles. Disable swiping articles in the listing to read them and remove the menu items to read all articles and to access sources settings.

Hide the favourite button in card items when accessing a public instance eeea5b1e06

Handle public access in the article reader screen 0dc511ebe6

Remove the favourite button from the article reader if accessing a public instance and don't mark articles as read when scrolling.

Handle public access in the article fragment c80177377c

Remove the button to read/unread articles

AmineL reviewed 2023-01-26 19:15:18 +00:00

androidApp/src/main/java/bou/amine/apps/readerforselfossv2/android/ReaderActivity.kt Outdated

						
				@ -85,3 +85,3 @@

				    private fun readItem(item: SelfossModel.Item) {

				        if (appSettingsService.isMarkOnScrollEnabled()) {

				        if (appSettingsService.isMarkOnScrollEnabled() and !appSettingsService.getPublicAccess()) {

AmineL commented

2023-01-26 19:15:18 +00:00

and should be changed to &&.

We do not care about the value of appSettingsService.getPublicAccess() if appSettingsService.isMarkOnScrollEnabled() is false.

`and` should be changed to `&&`. We do not care about the value of `appSettingsService.getPublicAccess()` if `appSettingsService.isMarkOnScrollEnabled()` is false.

👍 1

AmineL reviewed 2023-01-26 19:21:41 +00:00

androidApp/src/main/res/layout/card_item.xml Outdated

						
				@ -95,4 +94,3 @@

				                app:layout_constraintTop_toBottomOf="@+id/sourceTitleAndDate">

				                <ImageButton

				                    android:id="@+id/favButton"

AmineL commented

2023-01-26 19:21:41 +00:00

Why was the buttons order changed ?

davidoskky commented

2023-01-26 20:47:10 +00:00

The button order stays the same in the application as it currently is. This change allows the buttons to disappear dynamically and still align to the end of the layout.

AmineL marked this conversation as resolved

AmineL reviewed 2023-01-26 19:24:24 +00:00

shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/model/SelfossModel.kt Outdated

						
				@ -49,0 +51,4 @@

				        fun getApiConfiguration() = configuration ?: ApiConfiguration(null, null)

				    }

				    @kotlinx.serialization.Serializable

AmineL commented

2023-01-26 19:24:24 +00:00

Can be changed to @Serializable

Can be changed to `@Serializable`

👍 1

AmineL reviewed 2023-01-26 19:35:47 +00:00

shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/repository/RepositoryImpl.kt Outdated

						
				@ -449,0 +450,4 @@

				                }

				                if (appSettingsService.getUserName().isEmpty()

				                    && fetchedInformation.data.getApiConfiguration().isAuthEnabled()

				                    && fetchedInformation.data.getApiConfiguration().isPublicModeEnabled()) {

AmineL commented

2023-01-26 19:35:47 +00:00

I didn't look into how public instances work, but this condition seems weird.

isAuthEnabled and isPublicModeEnabled can be true at the same time ?

They seem to mean different things.

I didn't look into how public instances work, but this condition seems weird. `isAuthEnabled` and `isPublicModeEnabled` can be `true` at the same time ? They seem to mean different things.

davidoskky commented

2023-01-26 20:51:47 +00:00

Yes, it is the normal case in fact; the weird condition is actually isAuthEnabled false and isPublicModeEnabled true.
Authentication is enabled because the administrator user can log in and read/favourite articles or edit sources while everyone else can just read the articles (and not mark them as read). Having authentication disabled simply means that no password is required to access and thus everyone is administrator, in this case it would make no sense to enable public mode as well.

Yes, it is the normal case in fact; the weird condition is actually `isAuthEnabled` `false` and `isPublicModeEnabled` `true`. Authentication is enabled because the administrator user can log in and read/favourite articles or edit sources while everyone else can just read the articles (and not mark them as read). Having authentication disabled simply means that no password is required to access and thus everyone is administrator, in this case it would make no sense to enable public mode as well.

AmineL commented

2023-01-26 20:56:19 +00:00

Can you please add this to a comment ?

👍 1

AmineL reviewed 2023-01-26 19:36:46 +00:00

shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/repository/RepositoryImpl.kt

						
				@ -449,0 +451,4 @@

				                if (appSettingsService.getUserName().isEmpty()

				                    && fetchedInformation.data.getApiConfiguration().isAuthEnabled()

				                    && fetchedInformation.data.getApiConfiguration().isPublicModeEnabled()) {

				                    appSettingsService.updatePublicAccess(true)

AmineL commented

2023-01-26 19:36:46 +00:00

appSettingsService.updatePublicAccess(fetchedInformation.data.getApiConfiguration().isPublicModeEnabled()) isn't enough ?

`appSettingsService.updatePublicAccess(fetchedInformation.data.getApiConfiguration().isPublicModeEnabled())` isn't enough ?

davidoskky commented

2023-01-26 20:54:18 +00:00

No, public mode might be enabled but we could still log into the instance with username and password.
Public mode has limitations, thus if one had the credentials to access the instance, that would actually be the preferred option.

No, public mode might be enabled but we could still log into the instance with username and password. Public mode has limitations, thus if one had the credentials to access the instance, that would actually be the preferred option.

davidoskky commented

2023-01-27 13:01:54 +00:00

I'll rectify to explain what I mean. In the settings I am not storing the remote value of the configuration, I am directly storing whether we connected through public mode or not; in this way in any point of the application in which this information is required, that'll be readily available without any further processing. That's because we could connect trough the normal authentication method even if public mode was enabled.
Alternatively, I'd have to store both values of the remote configuration: isPublicModeEnabled and isAuthEnabled and each time check if these imply that we're connecting through public mode.
Since we are not using these information for anything else, storing it would be redundant.

I'll rectify to explain what I mean. In the settings I am not storing the remote value of the configuration, I am directly storing whether we connected through public mode or not; in this way in any point of the application in which this information is required, that'll be readily available without any further processing. That's because we could connect trough the normal authentication method even if public mode was enabled. Alternatively, I'd have to store both values of the remote configuration: `isPublicModeEnabled` and `isAuthEnabled` and each time check if these imply that we're connecting through public mode. Since we are not using these information for anything else, storing it would be redundant.

davidoskky added 1 commit 2023-01-26 23:25:36 +00:00

Cleaning ec0fd2faee

AmineL approved these changes 2023-01-28 10:24:47 +00:00

AmineL referenced this issue from a commit

2023-01-28 10:25:32 +00:00

feat: Handle public instances (#126)

AmineL merged commit f28e702549 into master

2023-01-28 10:25:34 +00:00

davidoskky deleted branch public

2023-01-28 12:12:39 +00:00

Sign in to join this conversation.

No reviewers

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Louvorg/ReaderForSelfoss-multiplatform#126