Project

General

Profile

Bug #5753

User Access Level in Settings ignored

Added by Flole Systems about 5 years ago. Updated about 5 years ago.

Status:
Fixed
Priority:
Normal
Assignee:
-
Category:
-
Target version:
-
Start date:
2019-10-19
Due date:
% Done:

0%

Estimated time:
Found in version:
latest master
Affected Versions:

Description

A recent commit broke the User Access Level. I have it set for a user to Expert but it stays at basic when the user logs in. Going through the recent commits now to find which one it is.

History

#1

Updated by Flole Systems about 5 years ago

Its dca55a1d393686c9ab1619f3c2e891685d40d428 causing the issues. I'm preparing a PR to kick out dca55a1d393686c9ab1619f3c2e891685d40d428 and depending commits again.

#2

Updated by Luis Alves about 5 years ago

Why not just fix the bug instead of removing the enhancements?

#3

Updated by Flole Systems about 5 years ago

Cause the "enhancements" cause a regression, so the obvious thing to do is remove them to fix the regression as fast as possible and have the author (or someone else) re-evaluate what went wrong there. This should have never been merged in the first place but seems like everyone who looked at it or tried it missed the issue its causing.

#4

Updated by Em Smith about 5 years ago

It is regrettable that you encountered problems. It would be useful if you can add some debug to identify the problem that you are having.

Features are merged to give wider testing, so I disagree that it "should never have been merged".

The feature was merged because people try their best to improve the program. It added functionality I will never use and for hardware I'll never own, but it was a requested feature that would be useful for many people with very low-memory devices such as TVs.

Occasionally regressions will happen.

The 4.2 branch is for people who want a stable, working system. This change was not merged to stable. If you use stable, you should rarely encounter a problem.

The master/4.3 branch is for people who wish to help test and add new features to ensure they are stable for the next major release. Sometimes there will be issues.

The feature was merged a week ago, and was also available for testing and code review as a PR for several months to allow testing in different environments since different people use different subsets of features, and the change mostly affected xmltv generation which has numerous downstream clients. My review and testing of the code didn’t identify a problem, and other people had not raised an issue. I re-reviewed the code and saw nothing obviously wrong, but since I am away I can not test.

#5

Updated by Flole Systems about 5 years ago

Features are merged to give wider testing, so I disagree that it "should never have been merged".

There should be testing before merging, it is in general a completely bad idea to just merge and hope you won't break anything. Usually there are unit/integration tests for that but in this case this is not the case so there should be extra caution when merging changes/touching the code.

The feature was merged because people try their best to improve the program.

Unfortunately this is not an improvement as it breaks a component everyone has to use (the Webinterface) just to allow some low memory devices (and users using this new feature also need to use the Webinterface at some point). Thats why this should be removed again until its fixed. The original author has usually (this might be different in this case!) no motivation to fix this issue as the change is merged, so unless someone else does the fix this will stay broken, this issue will move down the issue tracker until its forgotten and everyone will just accept that this doesnt work.

Occasionally regressions will happen.

And we need to fix them, otherwise we'll soon have a completely broken software which has some fancy new features but doesnt work at all.

If you use stable, you should rarely encounter a problem.

...except for the various problems that do exist in 4.2.

The master/4.3 branch is for people who wish to help test and add new features to ensure they are stable for the next major release. Sometimes there will be issues.

I know that, if you look at my commit history you'll see that I contributed aswell. Also I am not "mad" about this, I just reported my finding and I proposed a fix for this already and I am just waiting for it to be merged, meanwhile I apply my patch locally and users who have the same issue can use my PR. Not sure what gave you the impression that you need to explain to me what the different branches are for.

Last but not least this gives users a different access level than configured and renders the "force access level" useless, allowing users to access more than they should be able to.

#6

Updated by Em Smith about 5 years ago

The code was reviewed and tested by several people prior to merging. Everyone has different configurations and uses different subsets of functionality so sometimes problems are not caught until they are merged.

I re-tested the latest head with users with different UI levels and I don't have the problem that you have. The UI level is correctly persisted and set on login.

Perhaps other people can replicate your issue.

#7

Updated by Flole Systems about 5 years ago

That is indeed interesting that you are unable to replicate the issue. That means it's not completely broken, which unfortunately doesn't make it easier to find the issue.

When I did my debugging the websocket messages contained the "basic" access level even though it was set to "expert". After reverting mentioned commit and depending ones it was returning expert again. It could very well be that a specific access configuration is causing this, maybe when upgrading from older versions for example. I'll try to create a new user and see how that goes, I verified the users config file though and it looks alright to me.

#8

Updated by Joe User about 5 years ago

I am not sure exactly how the user's access configuration is saved and loaded, but since a new variable has been inserted, most likely loading an old config into new structure is the problem. Possibly just editing and making a small change for the user and saving will fix the discrepancy.

#9

Updated by Joe User about 5 years ago

I am not sure exactly how the user's access configuration is saved and loaded, but since a new variable has been inserted, most likely loading an old config into new structure is the problem. Possibly just editing and making a small change for the user and saving will fix the discrepancy.

 typedef struct access {    typedef struct access {
   char     *aa_username;      char     *aa_username;
   char     *aa_representative;      char     *aa_representative;
   char     *aa_lang;      char     *aa_lang;
   char     *aa_lang_ui;      char     *aa_lang_ui;
   uint32_t  aa_rights;      uint32_t  aa_rights;
   htsmsg_t *aa_profiles;      htsmsg_t *aa_profiles;
   htsmsg_t *aa_dvrcfgs;      htsmsg_t *aa_dvrcfgs;
   uint64_t *aa_chrange;      uint64_t *aa_chrange;
   int       aa_chrange_count;      int       aa_chrange_count;
   htsmsg_t *aa_chtags_exclude;      htsmsg_t *aa_chtags_exclude;
   htsmsg_t *aa_chtags;      htsmsg_t *aa_chtags;
   int       aa_match;      int       aa_match;
   uint32_t  aa_conn_limit;      uint32_t  aa_conn_limit;
  +uint32_t  aa_xmltv_output_format;
   uint32_t  aa_conn_limit_streaming;      uint32_t  aa_conn_limit_streaming;
   uint32_t  aa_conn_limit_dvr;      uint32_t  aa_conn_limit_dvr;
   uint32_t  aa_conn_streaming;      uint32_t  aa_conn_streaming;
   uint32_t  aa_conn_dvr;      uint32_t  aa_conn_dvr;
   int       aa_uilevel;      int       aa_uilevel;
   int       aa_uilevel_nochange;      int       aa_uilevel_nochange;
   char     *aa_theme;      char     *aa_theme;
   char     *aa_auth;      char     *aa_auth;
 } access_t;    } access_t;

#10

Updated by Jaroslav Kysela about 5 years ago

I cannot reproduce this problem, too. Because the structures are modified, it might be related to the partial rebuild. Have you tried to run 'make clean' before the build?

#11

Updated by Flole Systems about 5 years ago

I've pulled the new changes from today in and whatever the issue was seems to be fixed now again!

Thanks Jaroslav for the restructuring/modifications! Also thanks to everyone else who helped. We can mark this resolved now.

#12

Updated by Pablo R. about 5 years ago

Flole Systems wrote:

I've pulled the new changes from today in and whatever the issue was seems to be fixed now again !

Thanks Jaroslav for the restructuring/modifications! Also thanks to everyone else who helped. We can mark this resolved now.

Fixed!

#13

Updated by Mark Clarkstone about 5 years ago

  • Status changed from New to Fixed

Pablo R. wrote:

Flole Systems wrote:

I've pulled the new changes from today in and whatever the issue was seems to be fixed now again !

Thanks Jaroslav for the restructuring/modifications! Also thanks to everyone else who helped. We can mark this resolved now.

Fixed!

Also available in: Atom PDF