mantisbt icon indicating copy to clipboard operation
mantisbt copied to clipboard

Feature 22203: public / external access to tickets by unique URL

Open GunSmoker opened this issue 7 years ago • 18 comments

Implementation for http://www.mantisbt.org/bugs/view.php?id=22203 Screenshots of working feature can be found as attaches here: http://www.mantisbt.org/bugs/view.php?id=22203#c55198

  • The whole feature is disabled by default. It can be enabled by "$g_public_urls = ON;" in config_inc.php.
  • New "token" field was added to bugs table. Field is initially null, it is filled by randomized value on request.
  • Public Share URL can be copied from corresponding field in "Users monitoring issue" box when viewing bug; or it can be accessed when bug is created - via "View Public Share URL" button.
  • SOAP API was adjusted to return "token" for bug.
  • New bug_view_limited_inc.php (similar to bug_view_inc.php) was added, it implements a limited read-only view for access via Public Share URL.
  • Limited view shows description and steps to reproduce for public bugs only. Description/steps to reproduce is not shown for private bugs (only summary, resolution, version, dates are shown).
  • view.php switches between usual bug_view_inc.php (full view/edit) and bug_view_limited_inc.php (limited read-only view) - depending on URL (token is present and valid) and if user is already logged in.
  • Access is controlled by new global $g_login_by_bug_token, which is false for all usual operations. It is set to (int)reporter.id by view.php when valid token is present and bug_view_limited_inc.php is invoked.
  • authentication_api.php was adjusted to allow access when public share URL is used. Most changes are just extra safeguards.

GunSmoker avatar Jan 19 '17 12:01 GunSmoker

Thanks for your contribution.

I haven't had time to look into details or test your code, but I already see some issues which need to be addressed.

$g_public_urls config must be defined and documented (PHPDoc) in config_defaults_inc.php. Based on that, it will always be set, so you can get rid of all isset() calls; furthermore, config options must in principle be accessed via Config API (i.e. config_get() or config_get_global() depending on usage), and not directly. Consider also whether it should be added to $g_global_settings (I think yes), and/or to $g_public_config_names (probably not).

dregad avatar Jan 19 '17 13:01 dregad

Fixed code formatting, except for extra parenthesis. Your guidelines says it is allowed: "Add parentheses as appropriate to make things improve readability (even if not syntactically required by operators precedence)". Since I am not a PHP dev, I came from language with different operator priorities - I would like to leave this as it is.

GunSmoker avatar Jan 19 '17 17:01 GunSmoker

Maybe you would like to check out this branch to find something useful.

badfiles avatar Jan 19 '17 18:01 badfiles

@badfiles What exactly I should be looking at?

GunSmoker avatar Jan 19 '17 18:01 GunSmoker

Maybe you'll find something useful. Nothing specific comes to my mind.

badfiles avatar Jan 19 '17 18:01 badfiles

@badfiles Are there cat pictures or something?

GunSmoker avatar Jan 19 '17 18:01 GunSmoker

If someone could indicate what is wrong with passing tests - it would be greatly appreciated. I can not reproduce these locally.

GunSmoker avatar Jan 19 '17 19:01 GunSmoker

I can not reproduce these locally.

Meaning what, you can't run the PHPUnit tests ? Or they run but give different results ?

FYI, you can always review the Travis execution logs. In this case; the error is

1) IssueAddTest::testCreateIssue
SoapFault: Error Type: SYSTEM NOTICE,
Error Description: Trying to get property of non-object

The problem even be reproduced via the GUI, the error below occured when reporting a new issue:

SYSTEM NOTICE
'Trying to get property of non-object' in '/path/to/mantisbt/core/bug_api.php' line 856

When developing, you should use the recommended values for $g_display_errors, and set $g_show_detailed_errors = ON; if necessary.

dregad avatar Jan 20 '17 15:01 dregad

I have no time to have a deeper look at the moment, so just a few general comments.

  • changes in manage_user_page.php are not related to this PR
  • bug_view_limited_inc.php introduces quite a lot of redundant code (compare with bug_view_inc.php)
  • introduces some redundant code following if ( ( ON === config_get_global( 'public_urls' ) )

atrol avatar Jan 20 '17 18:01 atrol

@dregad

Meaning what, you can't run the PHPUnit tests ? Or they run but give different results ?

I tried to run phing. It gives: "Buildfile: build.xml does not exist!". I figure out phing is automation build tool, so I tried to run tests manually. Looking at travis_script.sh, I figure out I could run:

C:\PHP\php.exe C:\PHP\phpunit.phar --bootstrap ./tests/bootstrap.php ./tests/AllTests.php > tests\results.txt

Well, it runs, but does not show any errors. If I explicitly add some syntax bug - it will fail. Needless to say, reporting bugs manually via UI or submitting via SOAP API also works without any error.

I tested this on clean installation as well. Same result. PHP is configured for development, so display_errors is ON.

FYI, you can always review the Travis execution logs

Yes, but it does not show exception line, and it does not show call stack. So, pretty useless.

The problem even be reproduced via the GUI, the error below occured when reporting a new issue.

Thanks for that! It appears that your tests does not account DB changes? Otherwise, I do not understand why your bug table does not have new 'token' field. Or did I mess up with DB changes?

GunSmoker avatar Jan 21 '17 13:01 GunSmoker

@atrol

changes in manage_user_page.php are not related to this PR

These are not mine. I think I have messed up when trying to keep my clone up to date with ongoing commits to official Mantis. Sorry, I am not familiar with Git.

bug_view_limited_inc.php introduces quite a lot of redundant code (compare with bug_view_inc.php)

Do you mean file itself or its content? If it is about content - I thought that it is best to keep more code, even if it is not used, so one can simply enable it in the future, if one wants to show more info.

introduces some redundant code following if ( ( ON === config_get_global( 'public_urls' ) )

I could not understand why your Travis tests fails (see my reply above to dregad), so I probably added more safe-guards checks.

GunSmoker avatar Jan 21 '17 13:01 GunSmoker

@vboctor

Do we really need a db change vs. generate tokens from hashes.

Technically, I can use something like signature: token = SHA1(bugid + server-secret), but that means that we can't revoke access for specific URL / bug, if we will want to do that. DB field just offer more flexibility, even if it is not used right now. I occasionally used wiping out tokens in our practice, so I guess it is useful to have.

P.S. I already got some comments sent to me directly to e-mail, asking if it would be better to have per-user/per-bug tokens with ability to grant and revoke access. (seems like overkill to me)

Can we avoid having another issue view page vs. just configure the list of fields that should show up on the standard view issue page.

There are few reasons for standalone page.

First, is historic - I initially made isolated page, so I could upgrade Mantis more easily. But, of course, this is not the point for you.

Another reason: if you make bug_view_inc to display limited read-only view - then you will end up with "ugly" page which has a lot of blank spots. bug_view_limited_inc rearranges fields, to keep limited fields grouped together. See screenshots: http://www.mantisbt.org/bugs/view.php?id=22203

Of course, I can add more conditions and rearrange field in bug_view_inc. But I think that would add a lot of unnecessary complication.

Lastly, (in my opinion) a full featured editable work place and the limited read-only "screenshot" / "business card" has very little in common and could be significantly different visually. Even if it is not so right now.

I would like not to change the authentication code, but set the active user to a user with the appropriate access level (VIEWER). Such users would see pretty much all of the issue details excluding private notes for example.

How one can select user_id for that? Basically, I tried to change current/active user to reporter of bug report, but perhaps I did it wrong and there is a simpler way? I checked that current user comes from cookies, but is there a way to avoid that? Basically, I do not want to override cookies. For example, if you are already logged in, but do not have access to this specific bug, it would be nice to let you view bug via Public URL, but do not log you off.

Should we support share urls for private issues? Should it behave differently than public issues?

I see it that way: if one can see bug report, then nothing stops him from taking screenshot and share / post it. Public URLs is a similar thing, except it greatly reduces amount of exposed information. Currently I made just one the difference between private / public: private bugs does not show description and steps to reproduce (I assume it can contain sensitive info), public bugs do.

Here are two usage cases, from which this feature comes:

  1. Your software encounters bug;
  2. Your software submits bug to your bug tracker;
  3. Your software receives token;
  4. Your software thanks user and offer him a Public Share URL, so he can check when bug will be fixed.

Second:

  1. Your customer report issue with your software to your help desk;
  2. You confirm that this is a bug in your software;
  3. You add new bug to Mantis;
  4. You copy Public Share URL and give it to user, so he can check when bug will be fixed.

Considering these usage cases, I do not see that there must be some extra limitations. After all, you share bug with the person who actually reported it, there is no leak of info.

Anyway, if one does not like default rules - fine, just don't enable the feature, you will not lose anything. If one wants other rules - nothing stops one from altering this feature in the future to his liking (e.g. adding settings, etc.). I think it is better to have something working, instead of ideal feature which is not implemented (like votes in Mantis).

GunSmoker avatar Jan 21 '17 14:01 GunSmoker

@dregad

I think I have fixed the problem with tests. It was my mistake. Thanks for your help!

GunSmoker avatar Jan 21 '17 16:01 GunSmoker

Some thoughts:

  • The proposed implementation modifies authentication api, adding some special cases, that can be problematic with consistency, mantainability, etc
  • Adding fields to bug table, BugData structure... at first, i would avoid adding field there and keep those structures only to basic bug data. If neccessary, create a dedicated table, or use current token functionality, or look for a way to extend current tokens to allow linking to bugs.

Regarding the use of a separated view page, or integrating in current one:

  • We should avoid cluttering current view page with special cases to check for every element if it must be shown or not
  • Using a logged user to reuse current view page can be problematic. Victor suggested to create a dedicated user, like it's done with anonymous access. This dedicated user should ideally have "viewer" access. This user makes sense to be the same anonymous user. It has to be worked to allow this special case login even if anonymous login is disabled in the application.
  • A standalone page can make sense, where this page can act like a logged-out page. No navigation, no menus, and only show a the bug description is a very specific format.

I also wonder, if this could be implemented as a plugin. I think the only tricky part is the plugin page being accessed by unlogged users. If this is possible, the plugin can manage all the stuff for tokens, configuring what information to show, and how to retrieve it from the apis.

cproensa avatar Jan 22 '17 12:01 cproensa

@cproensa @vboctor

You do not like changes in auth API, which I can fully understand. However, you are suggesting to make analog of anonymous_account setting. But this setting is checked like everywhere, isn't it? I mean, even if I make this setting, I can not avoid altering auth API.

GunSmoker avatar Jan 22 '17 14:01 GunSmoker

I have removed safe-guard checks from auth API. Seems to work OK, but someone with a deep knowledge of auth API should really get a look.

GunSmoker avatar Jan 22 '17 15:01 GunSmoker

@GunSmoker I think re-factoring this as a plugin will drive the right behaviors. My feedback was heading in that direction, but I think re-factoring as a plugin will force the right design behaviors:

  • The plugin will own its own table for tokens.
  • The plugin will own its own the public (limited) view page.
  • No changes in core pages or core APIs.
  • No changes to core schema
  • Should be able to add a "Share" button to the View Issue page that creates/provides a token for the issue.
  • Can expose an API to be used by the app after creating an issue.
  • Installing the plugin == enabling the feature.

Future changes post v1 of the plugin can include:

  • You can enable UI for revoking a share URL, by creating a new token.
  • Customizations to what appears on the public view page.

If there is a plugin limitation that doesn't make this possible, we can address that by enhancing MantisBT core.

vboctor avatar Jan 23 '17 01:01 vboctor

@vboctor

I can understand why you do not want to add extra functionality to the Mantis core, which is already overloaded with options. However, I also think that you are seeing this from a wrong perspective.

Mantis has "Bug Tracker" in its name, which means it is supposed to be a specialized tool to report bugs. Did you saw two usage cases in https://github.com/mantisbt/mantisbt/pull/1001#issuecomment-274263740 ? Do you think it is "unusual" or "rare" usage cases for a bug tracker, so it should be offloaded to a plugin?

For example, if you develop desktop software, you usually have hundreds and thousands of users who are willing to send you a bug report. The simpler the send process is - the better. Ideally - just click a button. Creating extra user account on your bug tracker site definitely is not an option for these people, because most people will only report one or few bugs. Creating accounts works well when you have few people reporting many issues.

I think that the ability to send bug report and track its progress should be a core feature, on top of which other features may be build.

For example, bug tracker can receive e-mails and creates bug report for each incoming e-mail. It can send: "thank you for your report, you can track status of your report here: {public-link}" e-mail back to sender to confirm acceptance of the report and provide a way to monitor issue. You do not deal with accounts if you are communicating via e-mail. This is an existing feature in JIRA, Fogbugz, Redmine, Bugzilla, but not in Mantis (AFAIK). Additionally, bug tracker can create a limited "e-mail"/"anonymous" account assigned with sender's e-mail, so this person can get extra benefits when he "logs in" by following public link from e-mail. Such as seeing his other reports (from the same e-mail). This is called "community user account" in Fogbugz.

Another example: developer can enter any e-mail into "Monitor" edit and bug tracker will send a e-mail to this address (assuming no bug tracker's account exists with this e-mail): "This bug report may be interesting for you: {public-link}". This is called "Sharing" feature in JIRA.

And what about SOAP API? Can it be modified with a plugin? What about WSDL? Ability to get public link after submitting the bug is the whole reason for this feature.

P.S. BTW, there are two other important pieces which Mantis misses, but other bug trackers have: a technical alias (bug ID / bug hash) and count / occurencies. Which I also planned to add, as I tired of workarounds.

GunSmoker avatar Jan 23 '17 16:01 GunSmoker