Bug 28772 - Any user that can work with reports can see API keys of any other user
Summary: Any user that can work with reports can see API keys of any other user
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-07-28 07:07 UTC by Peter Vashchuk
Modified: 2021-09-30 07:52 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.04, 20.11.10, 20.05.16, 19.11.22


Attachments
Bug 28772: Make Koha::ApiKey->store encrypt the secret (6.68 KB, patch)
2021-08-27 16:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28772: Make Koha::ApiKey->store encrypt the secret (6.68 KB, patch)
2021-08-30 15:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28772: Update existing keys (1.51 KB, patch)
2021-08-30 15:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28772: Add a warning about hash_password usage in updatedatabase.pl (1.12 KB, patch)
2021-08-30 15:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28772: Display API secret once (4.19 KB, patch)
2021-08-30 15:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28772: Update existing keys [STABLE] (1.38 KB, patch)
2021-08-30 18:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28772: Make Koha::ApiKey->store encrypt the secret (6.75 KB, patch)
2021-09-03 15:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28772: Update existing keys (1.57 KB, patch)
2021-09-03 15:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28772: Add a warning about hash_password usage in updatedatabase.pl (1.19 KB, patch)
2021-09-03 15:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28772: Display API secret once (4.25 KB, patch)
2021-09-03 15:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28772: Update existing keys [STABLE] (1.45 KB, patch)
2021-09-03 15:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28722: Regression tests (1.66 KB, patch)
2021-09-09 11:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28772: Make secret validation use the new method (1.69 KB, patch)
2021-09-09 11:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28772: Do not hash secrets twice (1.13 KB, patch)
2021-09-09 11:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28772: (QA follow-up) Fix wrong message (959 bytes, patch)
2021-09-09 11:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28772: Update existing keys [STABLE] (1.52 KB, patch)
2021-09-09 11:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28772: Make Koha::ApiKey->store encrypt the secret (6.84 KB, patch)
2021-09-09 12:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28772: Update existing keys (1.67 KB, patch)
2021-09-09 12:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28772: Add a warning about hash_password usage in updatedatabase.pl (1.28 KB, patch)
2021-09-09 12:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28772: Display API secret once (4.35 KB, patch)
2021-09-09 12:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28722: Regression tests (1.75 KB, patch)
2021-09-09 12:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28772: Make secret validation use the new method (1.79 KB, patch)
2021-09-09 12:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28772: Do not hash secrets twice (1.23 KB, patch)
2021-09-09 12:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28772: (QA follow-up) Fix wrong message (1.03 KB, patch)
2021-09-09 12:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28772: Update existing keys [STABLE] (1.54 KB, patch)
2021-09-09 12:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28772: (QA follow-up) Apply change to other dbrev too [STABLE] (1.27 KB, patch)
2021-09-09 12:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28772: Make validate_secret return 1|0 (1.40 KB, patch)
2021-09-10 08:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28772: Fix Koha/Object.T (4.17 KB, patch)
2021-09-21 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28772: Fix Koha/Object.t (4.39 KB, patch)
2021-09-21 15:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28772: Fix auth_authenticate_api_request.t (1.02 KB, patch)
2021-09-21 16:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28772: Fix Koha/Object.t (4.31 KB, patch)
2021-09-21 16:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28772: [19.11.x] Fix Koha/Object.t (4.19 KB, patch)
2021-09-22 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Vashchuk 2021-07-28 07:07:03 UTC Comment hidden (obsolete)
Comment 1 Peter Vashchuk 2021-07-28 08:11:37 UTC
Any user with "Staff access, allows viewing of catalogue in staff interface (catalogue)" and "Create SQL reports (create_reports)" permissions can create the SQl report that will dump API keys from the db via custom SQL report, like: "SELECT * FROM `api_keys`". Also reports allow to dump and see hashed passwords, which I think is worth mentioning here but probably already known thing.

To reproduce:
1) create/pick existing patron and give them these permissions: "Staff access, allows viewing of catalogue in staff interface (catalogue)", "Create SQL reports (create_reports)" and "Execute SQL reports (execute_reports)";
2) go to the "Reports" -> "Create from SQL";
3) create report using this SQL: "SELECT * FROM `api_keys`";
4) run it, see how it dumps all API keys from the database.
Comment 2 Jonathan Druart 2021-07-28 08:57:07 UTC
I don't see any good solutions to fix that. It's a known issue, users with the report permission have access to the whole DB!

We could use temporary views, deny list (columns and/or tables), but you could always bypass it IMO.
Comment 3 Andrew Nugged 2021-07-28 09:02:11 UTC
One "partial" solution can be: to store hashed API keys like passwords (Joonas concluded in our brainstorm).

This of course requires more complicated changes to the interface and also backwards compatibility and so on, so let's develop this idea and see.
Comment 4 Andrew Nugged 2021-07-28 09:03:19 UTC Comment hidden (obsolete)
Comment 5 Andrew Nugged 2021-07-28 09:07:00 UTC
And also another part is documentation + even maybe "in-site interface text" on the "permissions assignment page" to explicitly warn admin-operator that adding "create reports" rights has consequences for that user to be able to dump API keys and hashed passwords, 

and then it's advised to assign "create reports" only to "superlibrarian level" trusty persons/accounts.

I.e. to give extra warning there for our users.
Comment 6 Tomás Cohen Arazi 2021-08-11 11:42:45 UTC
They should be hashed, for sure. Good catch!
Comment 7 Tomás Cohen Arazi 2021-08-27 16:56:08 UTC
Created attachment 124195 [details] [review]
Bug 28772: Make Koha::ApiKey->store encrypt the secret

This patch refactors the Koha::ApiKey class so:
- It encrypts the generated secret
- Allows accessing the plain text secret only immediately after the key
  creation (this implies that it won't be accessible if the key is
  fetched from the DB).
- It implements an allow list for attributes, that are not read only.
  Changing any other of them will make ->store throw an exception.
- A method for validating plain text secrets against the encrypted one
  is added.
- A method for accessing the plain text secret is added. Returns undef
  if the object is not 'fresh'.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/ApiKey.t
=> SUCCESS: Tests pass! Expected behavior is confirmed
3. Sign off :-D
Comment 8 Tomás Cohen Arazi 2021-08-30 15:18:00 UTC
Created attachment 124247 [details] [review]
Bug 28772: Make Koha::ApiKey->store encrypt the secret

This patch refactors the Koha::ApiKey class so:
- It encrypts the generated secret
- Allows accessing the plain text secret only immediately after the key
  creation (this implies that it won't be accessible if the key is
  fetched from the DB).
- It implements an allow list for attributes, that are not read only.
  Changing any other of them will make ->store throw an exception.
- A method for validating plain text secrets against the encrypted one
  is added.
- A method for accessing the plain text secret is added. Returns undef
  if the object is not 'fresh'.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/ApiKey.t
=> SUCCESS: Tests pass! Expected behavior is confirmed
3. Sign off :-D
Comment 9 Tomás Cohen Arazi 2021-08-30 15:18:06 UTC
Created attachment 124248 [details] [review]
Bug 28772: Update existing keys

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2021-08-30 15:18:12 UTC
Created attachment 124249 [details] [review]
Bug 28772: Add a warning about hash_password usage in updatedatabase.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2021-08-30 15:18:18 UTC
Created attachment 124250 [details] [review]
Bug 28772: Display API secret once

This patch makes the apikeys.pl display the generated API secret once,
when generated. After that, it won't be displayed by the UI.

To test:
1. Generate a new API key
=> FAIL: The secret is displayed in the API keys table
2. Visit some other page, and go back to the API keys page
=> FAIL: The API key secret is there
3. Apply this patch
4. Go to More > Manage API keys
=> SUCCESS: It no longer displays the secret
5. Generate a new API key
=> SUCCESS: The API key details (including the secret) are displayed.
=> SUCCESS: A message telling to copy the secret because it won't be
            displayed again is shown.
6. Repeat 4
=> SUCCESS: The secret is no longer displayed
7. Sign off :-D
Comment 12 Jonathan Druart 2021-08-30 15:30:19 UTC
Is this really a security bug fix? It was the current behavior and we now improve it, but does it deserve a backport into all stables (and so coordinate releases)?
Comment 13 Andrew Nugged 2021-08-30 16:30:53 UTC
The previous one (Bug 28759) for sure security fix: and it's simple, removes visibility for keys for other users except for owner and superlibrarian,

And the current one is more like "Security improvement",
but :) - I was concerned if to create this one publicly then it's revealing 28759 issue partially.

Jonathan: what to do with Bug 28759 then?
Comment 14 Tomás Cohen Arazi 2021-08-30 16:31:57 UTC
Besides that discussion, please test the patches promptly so we move forward.
Comment 15 Andrew Nugged 2021-08-30 16:33:51 UTC
Tomás: I thought, but you added more :P. I'll do it tomorrow
Comment 16 Tomás Cohen Arazi 2021-08-30 18:06:19 UTC
Created attachment 124251 [details] [review]
Bug 28772: Update existing keys [STABLE]

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Martin Renvoize 2021-09-03 15:17:30 UTC
Created attachment 124487 [details] [review]
Bug 28772: Make Koha::ApiKey->store encrypt the secret

This patch refactors the Koha::ApiKey class so:
- It encrypts the generated secret
- Allows accessing the plain text secret only immediately after the key
  creation (this implies that it won't be accessible if the key is
  fetched from the DB).
- It implements an allow list for attributes, that are not read only.
  Changing any other of them will make ->store throw an exception.
- A method for validating plain text secrets against the encrypted one
  is added.
- A method for accessing the plain text secret is added. Returns undef
  if the object is not 'fresh'.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/ApiKey.t
=> SUCCESS: Tests pass! Expected behavior is confirmed
3. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2021-09-03 15:17:34 UTC
Created attachment 124488 [details] [review]
Bug 28772: Update existing keys

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2021-09-03 15:17:38 UTC
Created attachment 124489 [details] [review]
Bug 28772: Add a warning about hash_password usage in updatedatabase.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2021-09-03 15:17:42 UTC
Created attachment 124490 [details] [review]
Bug 28772: Display API secret once

This patch makes the apikeys.pl display the generated API secret once,
when generated. After that, it won't be displayed by the UI.

To test:
1. Generate a new API key
=> FAIL: The secret is displayed in the API keys table
2. Visit some other page, and go back to the API keys page
=> FAIL: The API key secret is there
3. Apply this patch
4. Go to More > Manage API keys
=> SUCCESS: It no longer displays the secret
5. Generate a new API key
=> SUCCESS: The API key details (including the secret) are displayed.
=> SUCCESS: A message telling to copy the secret because it won't be
            displayed again is shown.
6. Repeat 4
=> SUCCESS: The secret is no longer displayed
7. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2021-09-03 15:17:46 UTC
Created attachment 124491 [details] [review]
Bug 28772: Update existing keys [STABLE]

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2021-09-03 15:19:53 UTC
Good catch, all works as expected. Signing off
Comment 23 Marcel de Rooy 2021-09-09 06:22:21 UTC
QAing
Comment 24 Marcel de Rooy 2021-09-09 07:01:03 UTC
ok( !$api_key->validate_secret( 'Wrong secret' ), 'Invalid secret returns true' );

Really true?
Comment 25 Marcel de Rooy 2021-09-09 07:01:40 UTC
(In reply to Martin Renvoize from comment #22)
> Good catch, all works as expected. Signing off

Are you sure? Did you test a converted API key in an API call?
Comment 26 Marcel de Rooy 2021-09-09 07:02:15 UTC
When we generate a CSRF token in Koha/Token.pm, we use Bytes::Random::Secure. Why not here? No blocker for now, but could these modules be moved closer to each other?

The sentence "Make sure to copy your API secret now. You won’t be able to see it again!" should be more prominently displayed. It should stand out much more. No blocker for now. Needs further attention imo.
Comment 27 Marcel de Rooy 2021-09-09 07:03:37 UTC
This is the most serious issue:

Update existing keys in dbrev does:
+            my $digest = Koha::AuthUtils::hash_password( $api_key->{secret} );
+            $sth->execute( $digest, $api_key->{client_id} );

Validate does:
    my $digest = Koha::AuthUtils::hash_password( $secret, $self->secret );
    return $self->secret eq $digest;

This does not seem to work. When you hashed, you used a new salt. You dont save it. When you check, you use the current secret as salt.
Looks to me that your eq test will almost never return true?
Comment 28 Marcel de Rooy 2021-09-09 07:33:35 UTC
Btw what about:

return (0, 'access_denied') unless $api_key->secret eq $client_secret;

In Koha/OAuth
Comment 29 Tomás Cohen Arazi 2021-09-09 11:06:05 UTC
(In reply to Marcel de Rooy from comment #27)
> This does not seem to work. When you hashed, you used a new salt. You dont
> save it. When you check, you use the current secret as salt.
> Looks to me that your eq test will almost never return true?

It works. Look at C4::Auth::checkpw_hash, which is broadly used in Koha:

sub checkpw_hash {
    my ( $password, $stored_hash ) = @_;

    return if $stored_hash eq '!';

    # check what encryption algorithm was implemented: Bcrypt - if the hash starts with '$2' it is Bcrypt else md5
    my $hash;
    if ( substr( $stored_hash, 0, 2 ) eq '$2' ) {
        $hash = hash_password( $password, $stored_hash );
    } else {
        $hash = md5_base64($password);
    }
    return $hash eq $stored_hash;
}
Comment 30 Tomás Cohen Arazi 2021-09-09 11:07:14 UTC
(In reply to Marcel de Rooy from comment #24)
> ok( !$api_key->validate_secret( 'Wrong secret' ), 'Invalid secret returns
> true' );
> 
> Really true?

Oops, copy and paste error on the test description, fixing.
Comment 31 Tomás Cohen Arazi 2021-09-09 11:22:40 UTC
(In reply to Marcel de Rooy from comment #28)
> Btw what about:
> 
> return (0, 'access_denied') unless $api_key->secret eq $client_secret;
> 
> In Koha/OAuth

This is the wrong bit. I forgot to tweak the oauth.t tests so it is passing the hash directly, which is wrong! Follow-up coming
Comment 32 Marcel de Rooy 2021-09-09 11:39:45 UTC
(In reply to Tomás Cohen Arazi from comment #29)
> (In reply to Marcel de Rooy from comment #27)
> > This does not seem to work. When you hashed, you used a new salt. You dont
> > save it. When you check, you use the current secret as salt.
> > Looks to me that your eq test will almost never return true?
> 
> It works. Look at C4::Auth::checkpw_hash, which is broadly used in Koha:
> 
> sub checkpw_hash {
>     my ( $password, $stored_hash ) = @_;
> 
>     return if $stored_hash eq '!';
> 
>     # check what encryption algorithm was implemented: Bcrypt - if the hash
> starts with '$2' it is Bcrypt else md5
>     my $hash;
>     if ( substr( $stored_hash, 0, 2 ) eq '$2' ) {
>         $hash = hash_password( $password, $stored_hash );
>     } else {
>         $hash = md5_base64($password);
>     }
>     return $hash eq $stored_hash;
> }

You dont convince me.
Look at the call in Auth:
$hash = hash_password( $password, $stored_hash );
But you do not use a stored hash in the db rev! You get a random one.
This really is different.

Please read the dbrev carefully :)
Comment 33 Tomás Cohen Arazi 2021-09-09 11:40:50 UTC
(In reply to Marcel de Rooy from comment #32)
> (In reply to Tomás Cohen Arazi from comment #29)
> > (In reply to Marcel de Rooy from comment #27)
> > > This does not seem to work. When you hashed, you used a new salt. You dont
> > > save it. When you check, you use the current secret as salt.
> > > Looks to me that your eq test will almost never return true?
> > 
> > It works. Look at C4::Auth::checkpw_hash, which is broadly used in Koha:
> > 
> > sub checkpw_hash {
> >     my ( $password, $stored_hash ) = @_;
> > 
> >     return if $stored_hash eq '!';
> > 
> >     # check what encryption algorithm was implemented: Bcrypt - if the hash
> > starts with '$2' it is Bcrypt else md5
> >     my $hash;
> >     if ( substr( $stored_hash, 0, 2 ) eq '$2' ) {
> >         $hash = hash_password( $password, $stored_hash );
> >     } else {
> >         $hash = md5_base64($password);
> >     }
> >     return $hash eq $stored_hash;
> > }
> 
> You dont convince me.
> Look at the call in Auth:
> $hash = hash_password( $password, $stored_hash );
> But you do not use a stored hash in the db rev! You get a random one.
> This really is different.
> 
> Please read the dbrev carefully :)

Haven't reached the DBRev in my fixes. Almost there.
Comment 34 Marcel de Rooy 2021-09-09 11:44:07 UTC
(In reply to Tomás Cohen Arazi from comment #33)
> (In reply to Marcel de Rooy from comment #32)
> > (In reply to Tomás Cohen Arazi from comment #29)
> > > (In reply to Marcel de Rooy from comment #27)
> > > > This does not seem to work. When you hashed, you used a new salt. You dont
> > > > save it. When you check, you use the current secret as salt.
> > > > Looks to me that your eq test will almost never return true?
> > > 
> > > It works. Look at C4::Auth::checkpw_hash, which is broadly used in Koha:
> > > 
> > > sub checkpw_hash {
> > >     my ( $password, $stored_hash ) = @_;
> > > 
> > >     return if $stored_hash eq '!';
> > > 
> > >     # check what encryption algorithm was implemented: Bcrypt - if the hash
> > > starts with '$2' it is Bcrypt else md5
> > >     my $hash;
> > >     if ( substr( $stored_hash, 0, 2 ) eq '$2' ) {
> > >         $hash = hash_password( $password, $stored_hash );
> > >     } else {
> > >         $hash = md5_base64($password);
> > >     }
> > >     return $hash eq $stored_hash;
> > > }
> > 
> > You dont convince me.
> > Look at the call in Auth:
> > $hash = hash_password( $password, $stored_hash );
> > But you do not use a stored hash in the db rev! You get a random one.
> > This really is different.
> > 
> > Please read the dbrev carefully :)
> 
> Haven't reached the DBRev in my fixes. Almost there.

Haha. If you didnt, how could you say that it works?
Comment 35 Tomás Cohen Arazi 2021-09-09 11:47:00 UTC
(In reply to Marcel de Rooy from comment #34)
> > Haven't reached the DBRev in my fixes. Almost there.
> 
> Haha. If you didnt, how could you say that it works?

Because, empirically, I saw it working. I'm submitting my follow-up with  a full test plan.
Comment 36 Marcel de Rooy 2021-09-09 11:50:24 UTC
(In reply to Tomás Cohen Arazi from comment #35)
> (In reply to Marcel de Rooy from comment #34)
> > > Haven't reached the DBRev in my fixes. Almost there.
> > 
> > Haha. If you didnt, how could you say that it works?
> 
> Because, empirically, I saw it working. I'm submitting my follow-up with  a
> full test plan.

Include a test with the converted API keys. Not only new keys..

I have the emperical experience that compact QA comments are not read very carefully. The emotion of seeing FQA is just too much :)
Comment 37 Tomás Cohen Arazi 2021-09-09 11:54:22 UTC
Created attachment 124691 [details] [review]
Bug 28722: Regression tests

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove tt/db_dependent/api/v1/oauth.t
=> FAIL: Tests fail!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 38 Tomás Cohen Arazi 2021-09-09 11:54:27 UTC
Created attachment 124692 [details] [review]
Bug 28772: Make secret validation use the new method

This patch makes the Koha::OAuth library use the new validation method

To test:
1. In master, enable RESTOAuth2ClientCredentials and have your
   superlibrarian patron a client_id/secret pair generated
2. Use Postman to gain an access token with the client_id/secret pair
=> SUCCESS: This works in Koha
3. Use the access token to GET /api/v1/patrons
=> SUCCESS: It works
4. Apply this patchset up to the regression tests
5. Run:
   $ updatedatabase
   $ koha-plack --restart kohadev
=> SUCCESS: All good
6. Repeat 2
=> FAIL: You get an error trying to acquire an access token. Boo
7. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: Tests fail!
8. Apply this patch
9. Run:
   $ koha-plack --restart kohadev
   $ kshell
  k$ prove t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests pass!
10. Repeat 2
=> SUCCESS: Your original client_id/secret pair works!
11. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 39 Tomás Cohen Arazi 2021-09-09 11:54:32 UTC
Created attachment 124693 [details] [review]
Bug 28772: Do not hash secrets twice

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 40 Tomás Cohen Arazi 2021-09-09 11:54:36 UTC
Created attachment 124694 [details] [review]
Bug 28772: (QA follow-up) Fix wrong message

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 41 Tomás Cohen Arazi 2021-09-09 11:57:30 UTC
Created attachment 124695 [details] [review]
Bug 28772: Update existing keys [STABLE]

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 42 Marcel de Rooy 2021-09-09 12:04:15 UTC
QA: Blocking this one in order to carefully examine the suggested improvements ;)
Comment 43 Marcel de Rooy 2021-09-09 12:37:47 UTC
(In reply to Marcel de Rooy from comment #27)
> This is the most serious issue:
> 
> Update existing keys in dbrev does:
> +            my $digest = Koha::AuthUtils::hash_password( $api_key->{secret}
> );
> +            $sth->execute( $digest, $api_key->{client_id} );
> 
> Validate does:
>     my $digest = Koha::AuthUtils::hash_password( $secret, $self->secret );
>     return $self->secret eq $digest;
> 
> This does not seem to work. When you hashed, you used a new salt. You dont
> save it. When you check, you use the current secret as salt.
> Looks to me that your eq test will almost never return true?

Found the answer: The first call without a salt returns a hash that includes the used salt. In the second call we include $self->secret, so in other words we provide bcrypt with the information to extract the salt from the first call.

Hope to remember that now ;)
Comment 44 Marcel de Rooy 2021-09-09 12:46:52 UTC
Created attachment 124696 [details] [review]
Bug 28772: Make Koha::ApiKey->store encrypt the secret

This patch refactors the Koha::ApiKey class so:
- It encrypts the generated secret
- Allows accessing the plain text secret only immediately after the key
  creation (this implies that it won't be accessible if the key is
  fetched from the DB).
- It implements an allow list for attributes, that are not read only.
  Changing any other of them will make ->store throw an exception.
- A method for validating plain text secrets against the encrypted one
  is added.
- A method for accessing the plain text secret is added. Returns undef
  if the object is not 'fresh'.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/ApiKey.t
=> SUCCESS: Tests pass! Expected behavior is confirmed
3. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Marcel de Rooy 2021-09-09 12:46:56 UTC
Created attachment 124697 [details] [review]
Bug 28772: Update existing keys

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 46 Marcel de Rooy 2021-09-09 12:47:01 UTC
Created attachment 124698 [details] [review]
Bug 28772: Add a warning about hash_password usage in updatedatabase.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 47 Marcel de Rooy 2021-09-09 12:47:05 UTC
Created attachment 124699 [details] [review]
Bug 28772: Display API secret once

This patch makes the apikeys.pl display the generated API secret once,
when generated. After that, it won't be displayed by the UI.

To test:
1. Generate a new API key
=> FAIL: The secret is displayed in the API keys table
2. Visit some other page, and go back to the API keys page
=> FAIL: The API key secret is there
3. Apply this patch
4. Go to More > Manage API keys
=> SUCCESS: It no longer displays the secret
5. Generate a new API key
=> SUCCESS: The API key details (including the secret) are displayed.
=> SUCCESS: A message telling to copy the secret because it won't be
            displayed again is shown.
6. Repeat 4
=> SUCCESS: The secret is no longer displayed
7. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 48 Marcel de Rooy 2021-09-09 12:47:10 UTC
Created attachment 124700 [details] [review]
Bug 28722: Regression tests

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove tt/db_dependent/api/v1/oauth.t
=> FAIL: Tests fail!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

https://bugs.koha-community.org/show_bug.cgi?id=28772

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 49 Marcel de Rooy 2021-09-09 12:47:14 UTC
Created attachment 124701 [details] [review]
Bug 28772: Make secret validation use the new method

This patch makes the Koha::OAuth library use the new validation method

To test:
1. In master, enable RESTOAuth2ClientCredentials and have your
   superlibrarian patron a client_id/secret pair generated
2. Use Postman to gain an access token with the client_id/secret pair
=> SUCCESS: This works in Koha
3. Use the access token to GET /api/v1/patrons
=> SUCCESS: It works
4. Apply this patchset up to the regression tests
5. Run:
   $ updatedatabase
   $ koha-plack --restart kohadev
=> SUCCESS: All good
6. Repeat 2
=> FAIL: You get an error trying to acquire an access token. Boo
7. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/oauth.t
=> FAIL: Tests fail!
8. Apply this patch
9. Run:
   $ koha-plack --restart kohadev
   $ kshell
  k$ prove t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests pass!
10. Repeat 2
=> SUCCESS: Your original client_id/secret pair works!
11. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 50 Marcel de Rooy 2021-09-09 12:47:18 UTC
Created attachment 124702 [details] [review]
Bug 28772: Do not hash secrets twice

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 51 Marcel de Rooy 2021-09-09 12:47:23 UTC
Created attachment 124703 [details] [review]
Bug 28772: (QA follow-up) Fix wrong message

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 52 Marcel de Rooy 2021-09-09 12:47:27 UTC
Created attachment 124704 [details] [review]
Bug 28772: Update existing keys [STABLE]

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 53 Marcel de Rooy 2021-09-09 12:47:31 UTC
Created attachment 124705 [details] [review]
Bug 28772: (QA follow-up) Apply change to other dbrev too [STABLE]

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 54 Marcel de Rooy 2021-09-09 12:55:42 UTC
Should we keep this one in security?
What about this report:
select * from oauth_access_tokens
Comment 55 Tomás Cohen Arazi 2021-09-09 17:25:05 UTC
(In reply to Marcel de Rooy from comment #54)
> Should we keep this one in security?
> What about this report:
> select * from oauth_access_tokens

Can we add oauth_access_tokens to some deny-list?
Comment 56 Jonathan Druart 2021-09-10 08:35:12 UTC
Created attachment 124727 [details] [review]
Bug 28772: Make validate_secret return 1|0

Not an empty string
Comment 57 Victor Grousset/tuxayo 2021-09-18 04:08:28 UTC
Hi, I'm having trouble backporting to 20.05.x due to a warning:

> root@e2231e847cad:koha(20.05.x-security)$ prove t/db_dependent/Koha/ApiKey.t
> t/db_dependent/Koha/ApiKey.t .. 1/4 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`api_keys`, CONSTRAINT `api_keys_fk_patron_id` FOREIGN KEY (`patron_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `api_keys` ( `client_id`, `description`, `patron_id`, `secret`) VALUES ( ?, ?, ?, ? )" with ParamValues: 0='48cbea53-9713-4316-829a-1ab9d435f31c', 1='a description', 2=62, 3='$2a$08$VRC3tfMUyQC.P.iuuVCax.pdfBmSY1gIKYMZI13gDK6iWOYlpX.FO'] at /kohadevbox/koha/Koha/Object.pm line 166
> t/db_dependent/Koha/ApiKey.t .. ok   
> All tests successful.
> Files=1, Tests=4,  3 wallclock secs ( 0.03 usr  0.00 sys +  2.29 cusr  0.31 csys =  2.63 CPU)
> Result: PASS

After more testing, it doesn't happen on the security branches for 19.11.x and 20.11.x
And it happens on 20.05.x whether I cherry-pick the patches from 19.11.x or 20.11.x

The atomic update is being applied in all cases.

Any hints?


I can still generate, remove, unrevoke and delete keys in the UI.
(no validity testing done though)
Comment 58 Jonathan Druart 2021-09-20 09:42:11 UTC
(In reply to Victor Grousset/tuxayo from comment #57)
> Hi, I'm having trouble backporting to 20.05.x due to a warning:
> 
> > root@e2231e847cad:koha(20.05.x-security)$ prove t/db_dependent/Koha/ApiKey.t
> > t/db_dependent/Koha/ApiKey.t .. 1/4 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`api_keys`, CONSTRAINT `api_keys_fk_patron_id` FOREIGN KEY (`patron_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `api_keys` ( `client_id`, `description`, `patron_id`, `secret`) VALUES ( ?, ?, ?, ? )" with ParamValues: 0='48cbea53-9713-4316-829a-1ab9d435f31c', 1='a description', 2=62, 3='$2a$08$VRC3tfMUyQC.P.iuuVCax.pdfBmSY1gIKYMZI13gDK6iWOYlpX.FO'] at /kohadevbox/koha/Koha/Object.pm line 166
> > t/db_dependent/Koha/ApiKey.t .. ok   
> > All tests successful.
> > Files=1, Tests=4,  3 wallclock secs ( 0.03 usr  0.00 sys +  2.29 cusr  0.31 csys =  2.63 CPU)
> > Result: PASS
> 
> After more testing, it doesn't happen on the security branches for 19.11.x
> and 20.11.x
> And it happens on 20.05.x whether I cherry-pick the patches from 19.11.x or
> 20.11.x
> 
> The atomic update is being applied in all cases.
> 
> Any hints?
> 
> 
> I can still generate, remove, unrevoke and delete keys in the UI.
> (no validity testing done though)

This warning is hidden since
  commit 82776d5a1202dec0a1eb13a93dceb9f01d79df7e
  Bug 26157: Hide expected DBI warnings from tests
Comment 59 Victor Grousset/tuxayo 2021-09-20 11:38:04 UTC
Great, no worries then.

Backported: Pushed to 20.05.x security branch for 20.05.16
Comment 60 Victor Grousset/tuxayo 2021-09-20 15:32:50 UTC
Bad news (or good, the tests are catching bugs), running the tests locally showed that these commits cause the following tests to fail:
(there are DBI Exceptions even without this patch but some are not the same)

root@b281b6624d26:koha(20.05.x-security)$ prove t/db_dependent/Koha/Object.t t/db_dependent/api/v1/auth_authenticate_api_request.t
t/db_dependent/Koha/Object.t ........................... 10/20 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`api_keys`, CONSTRAINT `api_keys_fk_patron_id` FOREIGN KEY (`patron_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `api_keys` ( `client_id`, `description`, `patron_id`, `secret`) VALUES ( ?, ?, ?, ? )" with ParamValues: 0='7b3051e0-02dc-42f7-8ac8-469d7108feec', 1='a description', 2=65, 3='$2a$08$ccvpjEufccH7g4aPY456OOdppMsEP9axhgJqI1vthjjGsy.bLWTWC'] at /kohadevbox/koha/Koha/Object.pm line 166
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`api_keys`, CONSTRAINT `api_keys_fk_patron_id` FOREIGN KEY (`patron_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `api_keys` ( `client_id`, `description`, `patron_id`, `secret`) VALUES ( ?, ?, ?, ? )" with ParamValues: 0='c565267c-4b6f-437f-956b-b610184d34e9', 1='a description', 2=65, 3='$2a$08$7.mn.j69xU/W0obkhx5FLOxY80PneJZKy2BvK3rJflhDfpZ2Cd.mS'] at /kohadevbox/koha/Koha/Object.pm line 166

    #   Failed test 'Exception is thrown correctly'
    #   at t/db_dependent/Koha/Object.t line 651.
    # expecting: Koha::Exceptions::Object::DuplicateID
    # found: Koha::Exceptions::Object::FKConstraint (Broken FK constraint)

    #   Failed test 'Exception message is correct'
    #   at t/db_dependent/Koha/Object.t line 654.
    #          got: 'Broken FK constraint'
    #     expected: 'Duplicate ID'
    # Looks like you planned 16 tests but ran 5.
    # Looks like you failed 2 tests of 5 run.

#   Failed test 'store() tests'
#   at t/db_dependent/Koha/Object.t line 729.
Can't locate object method "duplicate_id" via package "Koha::Exceptions::Object::FKConstraint" at t/db_dependent/Koha/Object.t line 660.
# Looks like your test exited with 255 just after 16.
t/db_dependent/Koha/Object.t ........................... Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 5/20 subtests 
t/db_dependent/api/v1/auth_authenticate_api_request.t .. 
    #   Failed test '200 OK'
    #   at t/db_dependent/api/v1/auth_authenticate_api_request.t line 69.
    #          got: '403'
    #     expected: '200'

    #   Failed test 'exact match for JSON Pointer "/expires_in"'
    #   at t/db_dependent/api/v1/auth_authenticate_api_request.t line 69.
    #          got: undef
    #     expected: '3600'

    #   Failed test 'exact match for JSON Pointer "/token_type"'
    #   at t/db_dependent/api/v1/auth_authenticate_api_request.t line 69.
    #          got: undef
    #     expected: 'Bearer'

    #   Failed test 'has value for JSON Pointer "/access_token"'
    #   at t/db_dependent/api/v1/auth_authenticate_api_request.t line 69.
Use of uninitialized value $access_token in concatenation (.) or string at t/db_dependent/api/v1/auth_authenticate_api_request.t line 82.

    #   Failed test '200 OK'
    #   at t/db_dependent/api/v1/auth_authenticate_api_request.t line 93.
    #          got: '401'
    #     expected: '200'

    #   Failed test 'The 'koha.user' object is defined in the stash'
    #   at t/db_dependent/api/v1/auth_authenticate_api_request.t line 96.

    #   Failed test 'userenv set correctly'
    #   at t/db_dependent/api/v1/auth_authenticate_api_request.t line 99.
    #          got: undef
    #     expected: '68'

    #   Failed test 'Interface correctly set to 'api''
    #   at t/db_dependent/api/v1/auth_authenticate_api_request.t line 100.
    #          got: 'opac'
    #     expected: 'api'
    # Looks like you planned 14 tests but ran 12.
    # Looks like you failed 8 tests of 12 run.
t/db_dependent/api/v1/auth_authenticate_api_request.t .. 1/3 
#   Failed test 'token-based tests'
#   at t/db_dependent/api/v1/auth_authenticate_api_request.t line 107.
t/db_dependent/api/v1/auth_authenticate_api_request.t .. 3/3 # Looks like you failed 1 test of 3.
t/db_dependent/api/v1/auth_authenticate_api_request.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/3 subtests
Comment 61 Victor Grousset/tuxayo 2021-09-20 19:21:46 UTC
The two tests fail also on security/21.05.04
Comment 62 Victor Grousset/tuxayo 2021-09-20 19:54:58 UTC
same failures for

security/new/security-release-20.11.10
security/19.11.22security
Comment 63 Victor Grousset/tuxayo 2021-09-20 20:01:27 UTC
same failures on master
Comment 64 Jonathan Druart 2021-09-21 09:00:56 UTC
Created attachment 125088 [details] [review]
Bug 28772: Fix Koha/Object.T

Koha::ApiKeys is no longer the simple object we need to test
Koha::Object->store, let use Koha::Cash::Register
Comment 65 Jonathan Druart 2021-09-21 09:01:46 UTC
Tomas, can you have a look at the other failure please?
Comment 66 Victor Grousset/tuxayo 2021-09-21 12:11:33 UTC
Thanks for the patch, unfortunately seems like there is a missing dependency for 20.05.x

root@c2ad25fd843a:koha(20.05.x-security)$ prove t/db_dependent/Koha/Object.t
t/db_dependent/Koha/Object.t .. 10/20     # No tests run!

#   Failed test 'No tests run for subtest "store() tests"'
#   at t/db_dependent/Koha/Object.t line 736.
Can't locate object method "new" via package "Koha::Cash::Register" (perhaps you forgot to load "Koha::Cash::Register"?) at t/db_dependent/Koha/Object.t line 619.
Comment 67 Jonathan Druart 2021-09-21 15:34:16 UTC
Created attachment 125112 [details] [review]
Bug 28772: Fix Koha/Object.t

Koha::ApiKeys is no longer the simple object we need to test
Koha::Object->store, let use Koha::Cash::Register
Comment 68 Jonathan Druart 2021-09-21 15:34:43 UTC
(In reply to Victor Grousset/tuxayo from comment #66)
> Thanks for the patch, unfortunately seems like there is a missing dependency
> for 20.05.x

Should pass with the amended patch.
Comment 69 Tomás Cohen Arazi 2021-09-21 16:09:13 UTC
(In reply to Jonathan Druart from comment #12)
> Is this really a security bug fix? It was the current behavior and we now
> improve it, but does it deserve a backport into all stables (and so
> coordinate releases)?

I'm not sure. It is a security bug because someone with reports permissions can impersonate other users through the API. I'm not sure it requires a coordinated release.
Comment 70 Tomás Cohen Arazi 2021-09-21 16:20:16 UTC
Created attachment 125116 [details] [review]
Bug 28772: Fix auth_authenticate_api_request.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 71 Tomás Cohen Arazi 2021-09-21 16:22:13 UTC
(In reply to Tomás Cohen Arazi from comment #70)
> Created attachment 125116 [details] [review] [review]
> Bug 28772: Fix auth_authenticate_api_request.t
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

The new behaviour is that the 'fresh' Koha::ApiKey object carries the generated secret that is accessible through the ->plain_text_secret method. The tests weren't updated to use it, so they were passing the encrypted secret, which was plain wrong.
Comment 72 Jonathan Druart 2021-09-21 16:34:01 UTC
Created attachment 125118 [details] [review]
Bug 28772: Fix Koha/Object.t

Koha::ApiKeys is no longer the simple object we need to test
Koha::Object->store, let use Koha::Library::Groups
Comment 73 Jonathan Druart 2021-09-21 16:35:39 UTC
With the last two patches those patches are ready to backported, please run the test suite locally. Tell us if you have conflicts when applying them on your respective branches.
Comment 74 Victor Grousset/tuxayo 2021-09-21 17:27:27 UTC
Thanks Jonathan and Tomás :) confirmed fix on 20.05.x

Will rerun the whole test suite and report in case of failures.
Comment 75 wainuiwitikapark 2021-09-22 07:56:34 UTC
I got conflicts when applying the last patch

and then got this:

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Koha/Object.t
t/db_dependent/Koha/Object.t .. 1/18 DBD::mysql::st execute failed: Unknown column 'branchprinter' in 'field list' [for Statement "INSERT INTO `branches` ( `branchaddress1`, `branchaddress2`, `branchaddress3`, `branchcity`, `branchcode`, `branchcountry`, `branchemail`, `branchfax`, `branchip`, `branchname`, `branchnotes`, `branchphone`, `branchprinter`, `branchreplyto`, `branchreturnpath`, `branchstate`, `branchurl`, `branchzip`, `geolocation`, `issuing`, `marcorgcode`, `opac_info`, `pickup_location`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? )" with ParamValues: 0='QADMdMlG', 1='VJKnXt9A', 2='HBsLJeAXM', 3='MYTdwc', 4='VaSzekoa5m', 5='Lq8VRIzrV', 6='OTliw_b', 7='h9uAsl8Ri', 8='MDfzjVnewk', 9='gDRQ0ocfvY', 10='GAVjikP', 11='AY4OtS2P', 12='MNo1lu_HgT6XpzrCdf_zC93daLsy_85itYY81l7GrK7lMglKS_B3Ui7LGlQtIG56JAZtCj', 13='zXfKBMZ0B7', 14='OWZluvG', 15='LbLhKT', 16='mXc_ngd', 17='RAdh0RXYzFtdHyZ5', 18='FO3p_UXnUe8QMRhN90T8_FxOYS46WTSRgBoYPSs0xjcpcUxGq5Wwg7uJKp_RaN82Zye0tLGp7rWkEEVikWvChBJT0iecYPrheOxTUWJ316jbBv0XJtZFFOWkwJC07XT6giITnCzhjXOYhbqSUm7zcCN6wXebIubdxqav5cs2ipWXrkfIJNqeUrvMDeSk3Iqk3yNPSJLHp2UI5FcQ_5jVRnzD5HZmRMvYxc8QAauXaanycEQ_qWmDkVwT7xvwN', 19=119, 20='NYSLw5qzUwMWbT4G', 21='oW_Pvl', 22=50] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.
    # No tests run!

#   Failed test 'No tests run for subtest "is_changed / make_column_dirty"'
#   at t/db_dependent/Koha/Object.t line 87.
DBIx::Class::Storage::DBI::_dbh_execute(): Unknown column 'branchprinter' in 'field list' at /home/vagrant/kohaclone/t/lib/TestBuilder.pm line 358
# Looks like your test exited with 255 just after 3.
t/db_dependent/Koha/Object.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 16/18 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/Object.t (Wstat: 65280 Tests: 3 Failed: 1)
  Failed test:  3
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 18 tests but ran 3.
Files=1, Tests=3,  2 wallclock secs ( 0.02 usr  0.01 sys +  1.15 cusr  0.13 csys =  1.31 CPU)
Result: FAIL
Comment 76 Jonathan Druart 2021-09-22 08:24:10 UTC
Created attachment 125128 [details] [review]
Bug 28772: [19.11.x] Fix Koha/Object.t

Koha::ApiKeys is no longer the simple object we need to test
Koha::Object->store, let use Koha::Library::Groups
Comment 77 Victor Grousset/tuxayo 2021-09-22 15:47:05 UTC
@Wainui are the relevant tests passing without the security patches?

Because unfortunately it's hard to have an environment where all the tests pass.
Like me and Kyle in koha-testing-docker have some tests failing in out branches (w/ and w/o security patches) and even master
  t/db_dependent/www/auth_values_input_www.t
  t/db_dependent/www/batch.t
  t/db_dependent/www/history.t
  t/Logger.t
And Fridolin have some other failing tests on their LXD based manual Koha installation.
Comment 78 Victor Grousset/tuxayo 2021-09-22 15:52:03 UTC
On my env when running the current security/19.11.22security branch, Koha/Object.t is passing.
It uses the comment 72 patch. And it also works when running «[19.11.x] Fix Koha/Object.t»
Comment 79 wainuiwitikapark 2021-09-22 23:42:54 UTC
The Object.t tests seem to still fail with or without the security patches

If it works on your end though can I assume it is okay and carry on do you think?
Comment 80 Jonathan Druart 2021-09-23 08:31:17 UTC
(In reply to wainuiwitikapark from comment #79)
> The Object.t tests seem to still fail with or without the security patches
> 
> If it works on your end though can I assume it is okay and carry on do you
> think?

Yes, the tests are passing for me. It would be good to investigate the issue you are facing however. Could you try and catch one of us on IRC? Or you can drop me an email with the output of the prove command if you prefer.
Comment 81 Jonathan Druart 2021-09-30 07:23:23 UTC
Backported to 21.05.04, 20.11.10, 20.05.16, 19.11.22
Comment 82 Jonathan Druart 2021-09-30 07:51:49 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 83 Jonathan Druart 2021-09-30 07:52:29 UTC
> Created attachment 124700 [details] [review] [review]
> Bug 28722: Regression tests

Wrong bug number, caught it too late..