| [19:19:13] | * jamesbond78 has joined #farmos |
| [19:20:04] | <jamesbond78> | hello |
| [19:20:19] | * AnasHaddad[m] has joined #farmos |
| [19:20:31] | * kunigunde[m] has joined #farmos |
| [19:20:49] | * farmtech[m] has joined #farmos |
| [19:21:01] | * EricLarese[m] has joined #farmos |
| [19:21:02] | * jamesbond78 has quit (Client Quit) |
| [19:21:14] | * postmanpat[m] has joined #farmos |
| [19:21:28] | * munjoma[m] has joined #farmos |
| [19:21:40] | * dazinism[m] has joined #farmos |
| [19:21:56] | * IyarkaiTechLab[m has joined #farmos |
| [19:22:10] | * Adam[m]123 has joined #farmos |
| [19:22:23] | * CarlosAlberto[m] has joined #farmos |
| [19:22:36] | * runfastthinkslow has joined #farmos |
| [19:22:48] | * nickhudson[m] has joined #farmos |
| [19:23:01] | * SpencerOnazi[m] has joined #farmos |
| [19:23:13] | * OmkarEkbote[m] has joined #farmos |
| [19:23:26] | * petednz[m] has joined #farmos |
| [19:23:40] | * and712[m] has joined #farmos |
| [19:23:59] | * Anonymous[m]1210 has joined #farmos |
| [19:24:12] | * davd[m] has joined #farmos |
| [19:24:24] | * frederike[m] has joined #farmos |
| [19:24:38] | * goldi[m] has joined #farmos |
| [19:24:51] | * gretel[m] has joined #farmos |
| [19:25:03] | * gunter[m] has joined #farmos |
| [19:25:22] | * harry[m] has joined #farmos |
| [19:25:35] | * hra38192639[m] has joined #farmos |
| [19:25:47] | * iuresearcherpw[m has joined #farmos |
| [19:25:59] | * mindcls[m] has joined #farmos |
| [19:26:12] | * olaf[m]1 has joined #farmos |
| [19:26:24] | * phantomse[m] has joined #farmos |
| [19:26:38] | * raul[m]1 has joined #farmos |
| [19:26:38] | * scrdcow[m] has joined #farmos |
| [19:26:51] | * skipper_is[m] has joined #farmos |
| [19:27:03] | * steinfarm[m] has joined #farmos |
| [19:27:16] | * AllanMacGregor[m has joined #farmos |
| [19:27:28] | * qoyyuum[m] has joined #farmos |
| [19:27:40] | * shane_aldrich[m] has joined #farmos |
| [19:27:52] | * toino[m] has joined #farmos |
| [19:28:04] | * aislinnpearson[m has joined #farmos |
| [19:28:17] | * JanSonntag[m] has joined #farmos |
| [19:28:37] | * ludwa6[m] has joined #farmos |
| [19:28:51] | * thattechguy[m] has joined #farmos |
| [19:29:04] | * Noaht[m] has joined #farmos |
| [19:29:16] | * ChristophWolfes[ has joined #farmos |
| [19:29:28] | * matrixtrix[m] has joined #farmos |
| [19:29:40] | * sgoodall[m] has joined #farmos |
| [19:29:56] | * oliverp44[m] has joined #farmos |
| [19:30:08] | * FreshiesFarmsLLC has joined #farmos |
| [19:30:21] | * RogerioMbuli[m] has joined #farmos |
| [19:30:34] | * MarcosCarballal[ has joined #farmos |
| [19:30:47] | * FeiWang[m] has joined #farmos |
| [19:30:59] | * UgeshB[m] has joined #farmos |
| [19:30:59] | * margeo[m] has joined #farmos |
| [19:31:11] | * donblair[m] has joined #farmos |
| [19:31:24] | * zackmuma[m] has joined #farmos |
| [19:31:41] | * justgav[m] has joined #farmos |
| [19:31:53] | * JustGav[m]1 has joined #farmos |
| [19:32:05] | * m035[m] has joined #farmos |
| [19:32:18] | * TheSlurpee[m] has joined #farmos |
| [19:32:32] | * EvanKelley[m] has joined #farmos |
| [19:32:44] | * ZaneBelkhayat[m] has joined #farmos |
| [19:32:56] | * elpronto[m] has joined #farmos |
| [19:33:11] | * dzfarmer[m] has joined #farmos |
| [19:33:24] | * courtneylking[m] has joined #farmos |
| [19:33:37] | * testuser769[m] has joined #farmos |
| [19:33:49] | * GuilhermePerotta has joined #farmos |
| [19:34:02] | * monkeyflowerfarm has joined #farmos |
| [19:34:15] | * botlfarm[m] has joined #farmos |
| [19:34:28] | * MattFletcher[m] has joined #farmos |
| [19:34:41] | * tool172[m] has joined #farmos |
| [19:34:56] | * ggcc18[m] has joined #farmos |
| [19:35:08] | * thattechguy99[m] has joined #farmos |
| [19:35:20] | * lauriewayne[m] has joined #farmos |
| [19:35:33] | * GudjonEinarMagnu has joined #farmos |
| [19:35:46] | * ThimmZwiener[m] has joined #farmos |
| [19:35:58] | * leku[m] has joined #farmos |
| [19:36:11] | * RafatKhashan[m] has joined #farmos |
| [19:36:24] | * nzsnowman[m] has joined #farmos |
| [19:36:37] | * sandg100[m] has joined #farmos |
| [19:36:49] | * r3c4ll[m] has joined #farmos |
| [19:37:01] | * eddieironsmith[m has joined #farmos |
| [19:37:13] | * GerardoLisboa[m] has joined #farmos |
| [19:37:26] | * JustinCampbell[m has joined #farmos |
| [19:37:38] | * ionitatelecom[m] has joined #farmos |
| [19:37:50] | * gbathree[m] has joined #farmos |
| [19:38:03] | * perfectinfoseeke has joined #farmos |
| [19:38:21] | * KarsonWynne[m] has joined #farmos |
| [19:38:33] | * BrandonSmith[m] has joined #farmos |
| [19:38:45] | * leogaggl[m] has joined #farmos |
| [19:38:58] | * ander[m] has joined #farmos |
| [20:58:40] | <symbioquine[m]> | <symbioquine[m]> "But on the other hand we have..." <- Any thoughts on this paul121 ? |
| [20:59:02] | <symbioquine[m]> | <symbioquine[m]> "For example, it's pretty..." <- RE ^ |
| [23:05:23] | <paul121[m]> | <symbioquine[m]> "Any thoughts on this paul121 ?" <- Huh, so naively I would think that `data.items` should reference all of the asset type schemas? But it seems like the weird thing is `data.items` is an object, and the `$ref` key is already defined so we can't simply define `data.items.$ref` multiple times for each... (full message at <https://libera.ems.host/_matrix/media/v3/download/libera.chat/01dd670462...) |
| [23:05:28] | <paul121[m]> | credit this SO: https://stackoverflow.com/questions/29781040/json-schema-possible-to-ref... |
| [23:05:57] | <paul121[m]> | I don't interact with json schema too much so this is pushing my knowledge haha |
| [23:07:22] | <paul121[m]> | and https://json-schema.org/understanding-json-schema/reference/combining.ht... |
| [23:43:32] | <symbioquine[m]> | > <@paul121:matrix.org> Huh, so naively I would think that `data.items` should reference all of the asset type schemas? But it seems like the weird thing is `data.items` is an object, and the `$ref` key is already defined so we can't simply define `data.items.$ref` multiple times for each asset type?... (full message at <https://libera.ems.host/_matrix/media/v3/download/libera.chat/7733892465...) |
| [23:51:20] | <symbioquine[m]> | <symbioquine[m]> "> <@paul121:matrix.org> Huh..." <- And maybe it actually already would work that way once https://www.drupal.org/project/jsonapi_schema/issues/3189930 is fixed; |
| [23:51:22] | <symbioquine[m]> | https://git.drupalcode.org/project/jsonapi_schema/-/blob/3cb2268798131cc... |
| [23:51:42] | <symbioquine[m]> | (with the `anyOf` composition, but same idea) |
| [23:59:28] | <paul121[m]> | ah yeah probably! |
| [00:01:04] | <symbioquine[m]> | Thanks for digging into the JSON:Schema weeds. I probably should have done it myself before asking... Just thought you might already have thought about that bit :) |
| [00:03:58] | <paul121[m]> | No worries. I haven't used the `resource/relationship` endpoints nor much of the json schema so it was a nice adventure |
| [00:05:54] | <paul121[m]> | > <@symbioquine:matrix.org> And maybe it actually already would work that way once https://www.drupal.org/project/jsonapi_schema/issues/3189930 is fixed; |
| [00:05:55] | <paul121[m]> | > https://git.drupalcode.org/project/jsonapi_schema/-/blob/3cb2268798131cc... |
| [00:05:55] | <paul121[m]> | it's weird the bug that's causing this is the type mismatch between `ResourceType` and `string`. it seems like that is a fundamental concept that would lead to other bugs |
| [00:06:39] | <symbioquine[m]> | I think the issue description addresses that... |
| [00:06:57] | <symbioquine[m]> | That most of the changes were made to switch to string, but that one was missed. |
| [00:07:31] | <symbioquine[m]> | ACTION uploaded an image: (23KiB) < https://libera.ems.host/_matrix/media/v3/download/matrix.org/KZujMHnpMAK... > |
| [00:07:43] | <symbioquine[m]> | Maybe that was just a guess ^ though |
| [04:00:17] | * kunigunde[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:17] | * farmtech[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:17] | * EricLarese[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:17] | * Adam[m]123 has quit (Quit: You have been kicked for being idle) |
| [04:00:17] | * postmanpat[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:17] | * munjoma[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:17] | * AnasHaddad[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:17] | * CarlosAlberto[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:17] | * dazinism[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:17] | * IyarkaiTechLab[m has quit (Quit: You have been kicked for being idle) |
| [04:00:17] | * runfastthinkslow has quit (Quit: You have been kicked for being idle) |
| [04:00:17] | * OmkarEkbote[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:17] | * nickhudson[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:17] | * SpencerOnazi[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:17] | * petednz[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:17] | * and712[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:17] | * Anonymous[m]1210 has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * gretel[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * hra38192639[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * gunter[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * phantomse[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * davd[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * iuresearcherpw[m has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * mindcls[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * raul[m]1 has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * frederike[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * goldi[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * harry[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * olaf[m]1 has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * skipper_is[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * steinfarm[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * AllanMacGregor[m has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * toino[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * qoyyuum[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * aislinnpearson[m has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * JanSonntag[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * matrixtrix[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * sgoodall[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * ludwa6[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * Noaht[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * ChristophWolfes[ has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * shane_aldrich[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * RogerioMbuli[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * oliverp44[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * MarcosCarballal[ has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * donblair[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * FreshiesFarmsLLC has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * thattechguy[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * FeiWang[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * UgeshB[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * JustGav[m]1 has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * EvanKelley[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * justgav[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * m035[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * zackmuma[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * elpronto[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * TheSlurpee[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * ZaneBelkhayat[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * testuser769[m] has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * GuilhermePerotta has quit (Quit: You have been kicked for being idle) |
| [04:00:18] | * dzfarmer[m] has quit (Quit: You have been kicked for being idle) |
| [09:44:33] | <mstenta[m]> | symbioquine paul121 should we commit the patch for that issue? |
| [09:58:58] | <mstenta[m]> | https://www.drupal.org/project/farm/issues/3189918#comment-14790663 |
| [10:01:09] | <symbioquine[m]> | Hmmm, just because I keep noticing it doesn't mean that it's blocking anyone... :) |
| [10:01:29] | <mstenta[m]> | Well it's been 2 years since I said "let's see what happens upstream" :-) |
| [10:01:49] | <mstenta[m]> | 🦗 |
| [10:01:49] | <symbioquine[m]> | haha, yeah |
| [10:02:21] | <symbioquine[m]> | I'm going to try applying your patch locally and see what happens |
| [10:02:48] | <mstenta[m]> | it's sooo simple - i do think that it was just missed during a refactor, now that i look at it again |
| [10:03:07] | <mstenta[m]> | but then again, that assumes it DID work as intended previously |
| [10:03:17] | <mstenta[m]> | so yea... let me know what you find |
| [10:05:43] | <symbioquine[m]> | Yep, that seems to work |
| [10:05:54] | <mstenta[m]> | great! |
| [10:06:04] | <mstenta[m]> | I'll add the patch to farmOS and close our tracking issue |
| [10:06:31] | <symbioquine[m]> | ACTION uploaded an image: (109KiB) < https://libera.ems.host/_matrix/media/v3/download/matrix.org/YSMQIJeYtnE... > |
| [10:06:47] | <symbioquine[m]> | The title is kind of weird... |
| [10:07:10] | <mstenta[m]> | yea that is weird |
| [10:07:10] | <symbioquine[m]> | > "Water assets, and Water assets" |
| [10:07:22] | <symbioquine[m]> | But it's doing the `.definitions.data.items.anyOf.*` part right |
| [10:07:48] | <symbioquine[m]> | The title is kind of cosmetic compared with that part |
| [10:07:58] | <mstenta[m]> | good good |
| [10:08:15] | <mstenta[m]> | yea i bet the title is generated somewhere more generally for schemas - curious what the logic is there |
| [10:14:42] | <symbioquine[m]> | Troubleshooting that title bit a little, then I'll try to post a comment on the upstream issue... |
| [10:24:25] | <symbioquine[m]> | ACTION sent a patch code block: https://libera.ems.host/_matrix/media/v3/download/libera.chat/a8ccf9f8e2... |
| [10:24:34] | <symbioquine[m]> | There's a second tiny bug in there :) |
| [10:24:58] | <symbioquine[m]> | Going to submit an updated patch on the upstream issue. |
| [10:25:37] | <mstenta[m]> | is that a different issue? |
| [10:26:16] | <symbioquine[m]> | Arguably, but they're both so tiny... |
| [10:26:20] | <symbioquine[m]> | Could go either way |
| [10:28:14] | <mstenta[m]> | I would open a second issue in the upstream repo, but we can add both patches to farmOS |
| [10:28:23] | <mstenta[m]> | If it's not the same exact error message |
| [10:28:28] | <symbioquine[m]> | Fair enough |
| [10:29:05] | <mstenta[m]> | They may end up wanting automated tests on both before anything gets merged (if anyone ever looks at it) |
| [10:29:54] | <mstenta[m]> | I'll add the first one's patch right now |
| [10:40:35] | <symbioquine[m]> | https://www.drupal.org/project/jsonapi_schema/issues/3322227#comment-147... |
| [10:43:20] | <symbioquine[m]> | And replied on https://www.drupal.org/project/jsonapi_schema/issues/3189930#comment-147... |
| [10:46:06] | <mstenta[m]> | Awesome! |
| [10:46:24] | <mstenta[m]> | I'll add that patch to farmOS too |
| [11:00:05] | * monkeyflowerfarm has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * MattFletcher[m] has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * tool172[m] has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * botlfarm[m] has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * ggcc18[m] has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * RafatKhashan[m] has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * thattechguy99[m] has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * lauriewayne[m] has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * ThimmZwiener[m] has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * GudjonEinarMagnu has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * nzsnowman[m] has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * sandg100[m] has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * eddieironsmith[m has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * leku[m] has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * r3c4ll[m] has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * GerardoLisboa[m] has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * JustinCampbell[m has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * perfectinfoseeke has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * KarsonWynne[m] has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * gbathree[m] has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * BrandonSmith[m] has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * ionitatelecom[m] has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * ander[m] has quit (Quit: You have been kicked for being idle) |
| [11:00:05] | * leogaggl[m] has quit (Quit: You have been kicked for being idle) |
| [11:32:01] | <paul121[m]> | does jsonapi_schema even have tests? It doesn't seem like it... I'm going to set to RTBC |
| [11:32:52] | <mstenta[m]> | ha good question |
| [11:54:03] | <symbioquine[m]> | Looks like the commit got created, but the Tweet/Toot didn't seem to go out: https://github.com/farmOS/farmOS-microblog/commit/29c723a67ad8e911f11184... https://fosstodon.org/@farmOS |
| [11:54:49] | <mstenta[m]> | Oh dangit! |
| [11:54:54] | <symbioquine[m]> | Guessing it has to do with commits created by one workflow not necessarily triggering another... |
| [11:55:03] | <mstenta[m]> | checking... |
| [11:55:54] | <mstenta[m]> | Oh I see what you mean yea... it did commit them |
| [11:55:57] | <mstenta[m]> | Womp |
| [11:56:40] | <symbioquine[m]> | https://stackoverflow.com/questions/73631383/triggering-a-workflow-run-a... |
| [11:56:51] | <mstenta[m]> | I will manually toot/tweet this one |
| [11:57:39] | <symbioquine[m]> | https://jahed.dev/2021/04/24/triggering-github-actions-from-commits-by-o... |
| [11:59:32] | <evered[m]> | is there a dev call this morning? |
| [12:00:06] | <mstenta[m]> | indeed! https://fosstodon.org/@farmOS/109360202102413686 |
| [12:00:11] | <evered[m]> | thank you |
| [12:36:44] | <mstenta[m]> | https://github.com/farmOS/farmOS.org/pull/63 |
| [13:41:49] | <mstenta[m]> | paul121 symbioquine sorry I had to jump off... did you find anything else on the client_secret stuff? |
| [13:42:06] | <mstenta[m]> | does it seem like we'll have to shim this in farmOS, at least temporarily? |
| [13:42:54] | <mstenta[m]> | (will it break ALL current integrations? aggregator? Farmer Ed 's node red stuff? Rothamsted python scripts? unknown other usages? etc?) |
| [13:45:25] | <paul121[m]> | we're still in the room if you want to join :-) |
| [13:45:52] | <paul121[m]> | ah Morgan may be leaving! |
| [13:48:07] | <paul121[m]> | tldr; although we set `client_secret = ''`, it is still setting a secret in the DB (hashing an empty string?) |
| [13:48:40] | <paul121[m]> | BUT if we set `client_secret = NULL` then everything works :-) |
| [13:49:15] | <paul121[m]> | I'm referring to this in `farm_api.install`: https://github.com/farmOS/farmOS/blob/03279969ab3dffc291b17033b801e2bff7... |
| [13:50:55] | <mstenta[m]> | oh! |
| [13:51:01] | <mstenta[m]> | interesting |
| [13:51:05] | <mstenta[m]> | so... it's just on the client creation? |
| [13:51:17] | <paul121[m]> | yes, it seems so |
| [13:51:28] | <mstenta[m]> | doesn't require changes to the code that's making requests? |
| [13:51:39] | <paul121[m]> | mstenta[m]: correct |
| [13:51:47] | <mstenta[m]> | ok so we should update the default farm client |
| [13:51:51] | <mstenta[m]> | and the surveystack aggregator client |
| [13:52:01] | <mstenta[m]> | will we need an update hook? |
| [13:52:07] | <mstenta[m]> | or just for NEW clients? |
| [13:52:15] | <paul121[m]> | although... symbioquine seemed to see that his default `farm` client *does have* a secret set in his install.. still using 5.0.6... which is interesting. |
| [13:53:07] | <mstenta[m]> | huh |
| [13:53:13] | <paul121[m]> | I think I have noticed another bug, however, where if you add a secret to the `farm` client via the UI, there is no way to clear the client secret (I've only tested this in 5.2.2) |
| [13:53:31] | <paul121[m]> | so it may be possible that Morgan set a client secret some time ago? |
| [13:53:38] | <mstenta[m]> | ah |
| [13:53:40] | <mstenta[m]> | hmm |
| [13:53:58] | <symbioquine[m]> | paul121[m]: I suppose it's possible |
| [13:54:18] | <symbioquine[m]> | Like you said on the call, I'd be curious what it looks like on other production installations... |
| [13:54:54] | <symbioquine[m]> | Presumably they should mostly be null for the installs that Farmier hosts right? |
| [13:55:27] | <mstenta[m]> | i can check some real quick |
| [13:55:33] | <mstenta[m]> | do you have a query off hand? |
| [13:55:42] | <symbioquine[m]> | Hmmm, just a sec... |
| [13:57:09] | <symbioquine[m]> | Maybe something like `SELECT * FROM public.consumer_field_data WHERE label = 'Farm default'`? |
| [13:58:27] | <symbioquine[m]> | `SELECT label, secret FROM public.consumer_field_data WHERE label = 'Farm default'` |
| [13:58:47] | <mstenta[m]> | i just sampled three and they all had secrets |
| [13:58:53] | <paul121[m]> | hmmm same on one of mine... |
| [13:58:57] | <mstenta[m]> | although... could it be that they are just encrypted empty strings? |
| [13:59:10] | <symbioquine[m]> | I mean that's the hypothesis... |
| [13:59:16] | <mstenta[m]> | they are different, but might be salted |
| [13:59:54] | <symbioquine[m]> | I wonder what happens if we change the curl request to pass '' as the client secret? |
| [14:03:20] | <paul121[m]> | it may be this line: https://git.drupalcode.org/project/simple_oauth/-/blob/5.2.x/src/Reposit... |
| [14:06:29] | <paul121[m]> | I'm trying to pass `client_secret=` and it doesn't work. When I passed `client_secret=''` in my debugger it showed up as a `"''"` |
| [14:06:49] | <paul121[m]> | * doesn't work, I get client authentication error. When |
| [14:08:38] | <paul121[m]> | <paul121[m]> "it may be this line: https://git..." <- ah of course... it is this line. specifically the `$client_secret && .... `, when `$client_secret = ''` it evaluates to FALSE |
| [14:08:54] | <paul121[m]> | ACTION sent a code block: https://libera.ems.host/_matrix/media/v3/download/libera.chat/2538e79dcf... |
| [14:09:56] | <mstenta[m]> | you were right paul121 - we should have added a test for a client without a secret :-) |
| [14:10:08] | <mstenta[m]> | would have caught this right away' |
| [14:11:53] | <paul121[m]> | yeaaa |
| [14:13:04] | <paul121[m]> | so... I'm trying to think what the solution is here |
| [14:14:01] | <paul121[m]> | <paul121[m]> "it may be this line: https://git..." <- That was added with this commit: https://git.drupalcode.org/project/simple_oauth/-/commit/ca09275e16969a1... |
| [14:14:01] | <paul121[m]> | This issue: https://www.drupal.org/project/simple_oauth/issues/3230707 |
| [14:18:22] | <paul121[m]> | aha okay I understand... let me grok.... |
| [14:25:37] | <paul121[m]> | * Two things changed from 5.0.x to 5.2.x in this commit: https://git.drupalcode.org/project/simple\_oauth/-/commit/ca09275e16969a14fd0d85754f547b8b70503ece |
| [14:25:37] | <paul121[m]> | 1. If a consumer has a client secret it is always validated, even if the client is non-confidential. This makes sense, but the issue is that our `farm` client had a secret which was the hashed empty string `client_secret = ''` :-( |
| [14:25:37] | <paul121[m]> | Relevant code: https://git.drupalcode.org/project/simple\_oauth/-/blob/5.2.x/src/Repositories/ClientRepository.php#L60-69 |
| [14:25:37] | <paul121[m]> | ...(truncated) |
| [14:25:37] | <paul121[m]> | Two things changed from 5.0.x to 5.2.x:... (full message at <https://libera.ems.host/_matrix/media/v3/download/libera.chat/51b0dbdc15...) |
| [14:27:53] | <mstenta[m]> | would this be considered a bug in simple_oauth? |
| [14:27:55] | <mstenta[m]> | still understanding |
| [14:28:06] | <mstenta[m]> | it's certainly a breaking change right? |
| [14:28:18] | <paul121[m]> | * Two things changed from 5.0.x to 5.2.x in this commit: https://git.drupalcode.org/project/simple\_oauth/-/commit/ca09275e16969a14fd0d85754f547b8b70503ece... (full message at <https://libera.ems.host/_matrix/media/v3/download/libera.chat/b185a50b06...) |
| [14:28:44] | <paul121[m]> | Right.... it's a breaking change, but yeah, I feel like simple_oauth won't really want to help us out with this? It's kinda our problem for using an empty string? |
| [14:29:07] | <mstenta[m]> | Is their code "correct" though? |
| [14:29:16] | <paul121[m]> | I mean there should be some validation to prevent storing the `client_secret` as an empty string, that is a bug that is 100% possible to do right now |
| [14:30:04] | <mstenta[m]> | Can we do an update hook to clear the `secret` if it is an empty string hashed? |
| [14:30:47] | <mstenta[m]> | Sorry hard to focus right now... might need to look at this in a more dedicated session later |
| [14:31:10] | <paul121[m]> | Yeah, I think that would be possible! |
| [14:32:17] | <mstenta[m]> | Would it fix the issue? |
| [14:32:25] | <mstenta[m]> | Without requiring changes on clients? |
| [14:32:51] | <paul121[m]> | They are using the Drupal core password checker interface: https://git.drupalcode.org/project/simple_oauth/-/blob/5.2.x/src/Reposit... |
| [14:32:51] | <paul121[m]> | https://git.drupalcode.org/project/simple_oauth/-/blob/5.2.x/src/Reposit... |
| [14:33:04] | <mstenta[m]> | Makes sense, so it's probably using the same salt |
| [14:33:28] | <mstenta[m]> | This is a unique case where we can test to see if a salted hashed password equals a known value :-) |
| [14:33:31] | <mstenta[m]> | (empty) |
| [14:33:38] | <paul121[m]> | I believe that would fix the issue. No changes for oauth clients, but changes for the consumer/client |
| [14:33:56] | <paul121[m]> | mstenta[m]: very unique! |
| [14:34:26] | <mstenta[m]> | So... an update hook to clear the secret field... and update modules that add a client so that it sets it to `NULL`? |
| [14:36:10] | <paul121[m]> | now.... there are two more general issues with 5.2.x: |
| [14:36:10] | <paul121[m]> | - If you provide a client secret via the UI, there is no way to "clear" that client secret via the UI.... |
| [14:36:10] | <paul121[m]> | - It appears there is a bug where the `openid` scope cannot be used with 5.2.x. We don't need this for farmOS core, but if people use farmOS as an OAuth server this might be a requirement (Morgan is). Open issue: https://www.drupal.org/project/simple_oauth/issues/3257293 |
| [14:36:48] | <mstenta[m]> | hmm |
| [14:37:28] | <mstenta[m]> | the first one sounds pretty minor... (should open an issue for it) i imagine the solution is: if the field is blank, set db col to `NULL`? |
| [14:41:10] | <symbioquine[m]> | mstenta[m]: The form field is always empty (unless a new value has been provided) since it never shows the existing secret - I think it actually can't since it's hashed (right?) |
| [14:41:39] | <symbioquine[m]> | You'd need something like a checkbox that means clear/reset the value |
| [14:42:12] | <paul121[m]> | Exactl |
| [14:42:21] | <paul121[m]> | * Exactly |
| [14:44:33] | <mstenta[m]> | Oh gotcha - didn't look |
| [14:44:51] | <mstenta[m]> | Maybe the form field should show `*****` if not `NULL! |
| [14:45:00] | <mstenta[m]> | s/!/`/ |
| [14:45:10] | <mstenta[m]> | Meh... checkbox might be better |
| [14:46:09] | <mstenta[m]> | Anyway, that's annoying and def an upstream, but not super in-scope for us right now |
| [14:46:12] | <mstenta[m]> | symbioquine: can you sanity check this for me when you have a moment? https://github.com/farmOS/farmOS-microblog/pull/2 |
| [14:47:17] | <symbioquine[m]> | I guess we're assuming GitHub Actions already have the ssh hostkeys trusted? |
| [14:47:25] | <symbioquine[m]> | (seems like a reasonable assumption) |
| [14:47:49] | <mstenta[m]> | oh you mean so it doesn't get hung up on that stupid "yes/no" prompt? |
| [14:48:04] | <mstenta[m]> | (it's not stupid... it's good. i just always get hung up on it myself lol) |
| [14:48:17] | <symbioquine[m]> | yeah |
| [14:48:34] | <mstenta[m]> | well... if it doesn't work, then the action will fail... so worth a test? |
| [14:49:10] | <symbioquine[m]> | Just reading up on that `ssh-key` argument. I don't think I've used that before... |
| [14:50:01] | <symbioquine[m]> | I guess baked into that change is the assumption that checking out the repository with that key will subsequently enable future pushes from the checked out repository using the same key. |
| [14:50:13] | <symbioquine[m]> | I don't know whether that's true. |
| [14:50:54] | <symbioquine[m]> | It's a little different than the strategy I'm using [here](https://symbioquine.net/2022-09-28-github-actions-drupal-module-build-st...) where I explicitly add the key to ssh-agent. |
| [14:51:29] | <mstenta[m]> | `ssh-key` is what he used in https://jahed.dev/2021/04/24/triggering-github-actions-from-commits-by-o... |
| [14:51:47] | <symbioquine[m]> | ah! |
| [14:52:44] | <symbioquine[m]> | Cool, I guess it can't hurt to try it :) |
| [14:54:13] | <mstenta[m]> | Merged... we'll see next week if it worked :-) |
| [14:54:26] | <mstenta[m]> | (Gives me a reason to post more often while debugging haha) |
| [14:54:37] | <symbioquine[m]> | and now we wait :) |
| [14:56:12] | <mstenta[m]> | if this works then it will clear the way for https://github.com/farmOS/farmOS/commit/6712a5a7eef49dea5ee4bf7baf72a888... (using deploy key instead of PAT) |
| [14:56:49] | <mstenta[m]> | so maybe we should wait until next thursday to tag a new release :-) |
| [14:57:02] | <symbioquine[m]> | Hmmm |
| [14:57:10] | <mstenta[m]> | (pending this bug getting fixed, and the migration one that paul121 and I started) |
| [14:59:06] | <mstenta[m]> | gotta go pick up kids 👋 |
| [15:11:22] | <mstenta[m]> | paul121: could the empty hash check happen in simple_oauth? |
| [15:11:58] | <symbioquine[m]> | mstenta[m]: I was wondering the same thing |
| [15:15:44] | <mstenta[m]> | In the empty check |
| [15:15:53] | <mstenta[m]> | I think that would also essentially fix the form issue, right? |
| [15:25:35] | <symbioquine[m]> | I think so |
| [15:58:41] | <paul121[m]> | hmm where would simple oauth check that? |
| [16:24:48] | <mstenta[m]> | Here? |
| [16:25:31] | <mstenta[m]> | Here? |
| [16:25:46] | <mstenta[m]> | Hmm Element android wont let me quote |
| [16:26:09] | <mstenta[m]> | In the lines you pointed to paul121 |
| [16:26:25] | <mstenta[m]> | Where it checks if the secret is empty |
| [16:26:48] | <mstenta[m]> | Seems like it could also check to see if the secret matches a hashed empty string |
| [16:28:10] | <paul121[m]> | Oh true. But ideally it wouldn't need to run every time? |
| [16:28:59] | <mstenta[m]> | What wouldn't? |
| [16:29:39] | <paul121[m]> | Checking for the hashed empty string on every API request |
| [16:29:41] | <paul121[m]> | seems like we should just not allow empty string :-) |
| [16:29:56] | <mstenta[m]> | Right, but it did |
| [16:30:20] | <paul121[m]> | I like your idea of the update hook |
| [16:30:51] | <mstenta[m]> | This was a breaking change |
| [16:31:07] | <mstenta[m]> | Yea, that works too... in simple_oauth tho or farmOS? |
| [16:31:14] | <mstenta[m]> | I feel like simple_oauth needs to take some responsibility here too |
| [16:31:43] | <paul121[m]> | I don't care where :P |
| [16:31:56] | <paul121[m]> | haha yeah I agree |
| [16:32:07] | <paul121[m]> | looking to see if the oauth spec says anything about empty strings.. |
| [16:32:59] | <mstenta[m]> | If a quick fix is all we want then we could open an issue on simple_oauth and upload a patch that just adds an additional check for hashed empty string |
| [16:33:28] | <mstenta[m]> | Not as the long term solution, but just to raise attention to it and give us a workaround to unblock us |
| [16:34:10] | <mstenta[m]> | Regardless of what oauth spec is, if this is a breaking change they need to document that |
| [16:34:29] | <mstenta[m]> | And maybe it goes away in 6x |
| [16:37:40] | <paul121[m]> | Instead of a patch I would prefer we just add that update hook ourselves. It would be fine if simple oauth later added the same update hook, it just wouldn't do anything |
| [16:38:46] | <mstenta[m]> | Hmm |
| [16:40:06] | <mstenta[m]> | Minor but: the benefit of a patch is it goes away. Update hooks remain in the code footprint |
| [16:42:26] | <mstenta[m]> | > Oh true. But ideally it wouldn't need to run every time? |
| [16:42:26] | <mstenta[m]> | Is the concern here that it adds performance overhead? |
| [16:43:02] | <mstenta[m]> | Its already running the other logic every time |
| [16:43:03] | <paul121[m]> | kinda, its just unnecessary |
| [16:43:38] | <mstenta[m]> | Yea. Its only necessary because they allowed hashed empty strings to be saved in the first place |
| [16:44:22] | <mstenta[m]> | It does seem like simple_oauth should add an update hook to fix it, and fix the form at the same time |
| [16:44:35] | <mstenta[m]> | And entity save if that is causing it |
| [16:44:56] | <mstenta[m]> | All of this just leads me back to: |
| [16:45:40] | <mstenta[m]> | > Not as the long term solution, but just to raise attention to it and give us a workaround to unblock us |
| [16:45:44] | <mstenta[m]> | They need to fix it in 6.x regardless |
| [16:46:04] | <mstenta[m]> | So maybe we just have a simple patch until then |
| [16:49:05] | <paul121[m]> | It just seems like we could do the long term solution now.. I'd prefer that over maintaining a patch, but yes that would work |
| [16:49:37] | <mstenta[m]> | Yea I agree - it's just that the long term solution is in simple_oauth, not in farmOS |
| [16:50:01] | <mstenta[m]> | It bothers me to add an update hook for someone else's issue |
| [16:50:33] | <mstenta[m]> | Minor, I know... just don't like it :-) |
| [16:51:19] | <mstenta[m]> | Just so I understand: will we need to change the surveystack module (and any others that added a consumer in hook_install())? |
| [16:51:34] | <mstenta[m]> | Seems like it should "just work" without changing them |
| [16:51:52] | <mstenta[m]> | (again pointing to the fact that this is simple_oauth's issue, not ours?) |
| [16:52:22] | <mstenta[m]> | like... i guess i would expect that an "empty secret" is the same as "no secret"... right?? haha |
| [16:54:38] | <paul121[m]> | well what is interesting is that the `consumer.secret` field is a core `Password` field type... so that field must allow the difference between NULL and `''`, not sure why |
| [16:55:11] | <paul121[m]> | but yes, I agree they should work the same as they did before |
| [16:57:42] | <mstenta[m]> | well so... what do you think about 1) opening an issue in simple_oauth, 2) creating a temporary patch (maybe just for 5.2.x) that just checks for empty hashed, 3) propose the proper solution (maybe just for 6.x) that adds an update hook and proper logic to equate NULL and empty |
| [16:58:41] | <mstenta[m]> | no doubt 3 will take some time for upstream maintainers to think about etc... 2 would allow us to release beta8 |
| [16:59:12] | <mstenta[m]> | could even make a patch for 3 right after 2, start the issue for 5.2.x, and then change it to 6.x for the new patch |
| [16:59:41] | <mstenta[m]> | (trying to think strategically here to both unblock us in the short term and solve this properly in the long term, with minimal code in farmOS) |
| [17:00:33] | <mstenta[m]> | (i guess i also don't really know: is 6.x even using the same code now?) |
| [17:00:43] | <paul121[m]> | That all sounds good |
| [17:00:53] | <paul121[m]> | q: do you want to change all of our `hook_install` to use `NULL` right now? |
| [17:01:05] | <paul121[m]> | I think we should - because that seems more correct |
| [17:01:16] | <mstenta[m]> | i suppose we could in farmOS yes |
| [17:04:25] | <paul121[m]> | so for #2, what logic do we put here: https://git.drupalcode.org/project/simple_oauth/-/blob/5.2.x/src/Reposit... |
| [17:08:57] | <mstenta[m]> | Maybe...... (full message at <https://libera.ems.host/_matrix/media/v3/download/libera.chat/539fa3752d...) |
| [17:09:41] | <mstenta[m]> | could shuffle that around to make it even cleaner probably |
| [17:11:32] | <paul121[m]> | hmm yes and we might need to modify the code block above this too... because of the `client_credentials` edge case |
| [17:12:16] | <mstenta[m]> | ah yea... so maybe a block above both that sets a new `$secret_is_empty` boolean, which can be used in both below |
| [17:13:10] | <mstenta[m]> | `$secret_is_empty = $secret_field->isEmpty() || $this->passwordChecker->check('', $client_drupal_entity->get('secret')->value)` |
| [17:14:03] | <paul121[m]> | hmmm... if we include this patch we should really have tests run against it... |
| [17:14:17] | <mstenta[m]> | in our code? agreed. |
| [17:14:40] | <mstenta[m]> | > you were right paul121 - we should have added a test for a client without a secret :-) |
| [17:15:09] | <paul121[m]> | but we basically need to run all of the simple_oauth tests... this validation logic is pretty important for everything |
| [17:16:14] | <mstenta[m]> | does d.o handle that for simple_oauth now when a patch is uploaded? |
| [17:16:28] | <mstenta[m]> | would that cover the overall tests? |
| [17:17:28] | <paul121[m]> | It should. I think tests are running on gitlab now too |
| [17:18:46] | <paul121[m]> | Yeah I see 5.2.x pipelines there |
| [17:18:53] | <mstenta[m]> | cool |
| [17:34:58] | <symbioquine[m]> | There's an argument for having defensive tests as part of the farmOS codebase. Tests in the decencies codebase would be updated along with the functionality so they wouldn't help against intentional changes. |
| [17:37:36] | <paul121[m]> | how does this look?... (full message at <https://libera.ems.host/_matrix/media/v3/download/libera.chat/5e3a25bfd3...) |
| [17:37:42] | <paul121[m]> | * how does this look?... (full message at <https://libera.ems.host/_matrix/media/v3/download/libera.chat/22eeb272aa...) |
| [17:39:46] | <paul121[m]> | * how does this look?... (full message at <https://libera.ems.host/_matrix/media/v3/download/libera.chat/d05041f407...) |
| [17:48:23] | <mstenta[m]> | LGTM! |
| [18:51:08] | * summonner has joined #farmos |
