| [20:00:30] | <mstenta[m]> | Just added `'client_secret' => $this->clientSecret,` after this line: https://github.com/farmOS/farmOS/blob/8a2dd9474b4b24cf0cad9cb751edd83405... |
| [20:00:48] | <mstenta[m]> | I'm still not certain why this test started failing in the first place though... |
| [20:01:49] | <mstenta[m]> | > maybe they started using client_secret in the upstream tests |
| [20:01:49] | <mstenta[m]> | as far as i can tell from git blame history they've used `client_secret` from the beginning in their test class... so i'm actually not sure why our tests EVER passed... but *something* must have changed in `simple_oauth` (or the upstream oauth lib)... |
| [20:04:08] | <mstenta[m]> | good news is this suggests that it was just the tests that weren't working, but in practice the API still works normally (i thinK) |
| [21:45:34] | <mstenta[m]> | Tests are passing! đ https://www.drupal.org/project/farm/issues/3282186#comment-14713816 |
| [21:49:44] | <mstenta[m]> | PHP 8 tests are passing too! https://github.com/mstenta/farmOS/actions/runs/3155675638 |
| [22:58:16] | <paul121[m]> | Nice mstenta!!... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/4e1a84653c...) |
| [22:58:32] | <paul121[m]> | * Nice mstenta!!... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/44d9ca871a...) |
| [23:01:46] | <paul121[m]> | I also feel pretty confident that this won't affect current API/OAuth integrations |
| [23:02:16] | <paul121[m]> | Although it also makes me think that we don't have sufficient test coverage for clients that are not configured with a `client_secret` |
| [23:04:04] | <paul121[m]> | Ummm but a larger issue I've uncovered while digging in the simple OAuth issue queue... ugh **In 6.x they have removed the password credentials grant** |
| [23:06:03] | <paul121[m]> | There were two relevant issues, neither had any discussion |
| [23:06:03] | <paul121[m]> | - https://www.drupal.org/project/simple_oauth/issues/3277730 |
| [23:06:03] | <paul121[m]> | - https://www.drupal.org/project/simple_oauth/issues/3173947 |
| [23:08:39] | <paul121[m]> | It seems kinda frustrating. But I'm tired and maybe extra prone to frustration so I'll leave feelings aside.. hah. |
| [23:12:33] | <paul121[m]> | Yes, we shouldn't be typing our username/passwords for temporary use in other apps, scripts, etc. Field kit should really be using the authorization flow as well. But as far I understand this still is a part of the OAuth spec, and the server admin should be allowed to configure this... |
| [23:16:58] | <paul121[m]> | I'm curious what you all think, especially those of you that tinker with farmOS API. |
| [23:16:59] | <paul121[m]> | Without the password credentials flow you will either have to 1) implement the authorization flow into your projects, or 2) use the client credentials grant with a private client_id/client_secret pair. 2 is essentially a username/password but most notably this client can only be associated with a *single* user. |
| [23:19:58] | <paul121[m]> | I should also say the OAuth grant types are implemented in a plugin system, so a password credentials grant is likely something we could provide with farmOS, while still using simple OAuth v6 |
| [23:21:25] | <paul121[m]> | But.. otherwise this is kinda a breaking change with no upgrade path :-/ |
| [23:23:44] | <paul121[m]> | Available grant types in 6.x branch: https://git.drupalcode.org/project/simple_oauth/-/tree/6.0.x/src/Plugin/... |
| [01:47:23] | <FarmerEd[m]> | <paul121[m]> "Yes, we shouldn't be typing..." <- Honestly password grant is what I've been using for authentication with the API, so the selfish part of me would like to keep it đ. However everything I use so far uses the same authentication flow so it should be only one minor fix for me to change. The other applications I use Aauth2 for don't allow password grant either. |
| [01:49:50] | <FarmerEd[m]> | I think the Node-Red nodes I published are hardcoded for password grant, but that probably wasn't a great idea to start with. |
| [01:58:20] | <paul121[m]> | Interesting that others don't allow password grant either! Yeah I'm curious with Node-Red if & how the authorization flow would work.. |
| [02:10:46] | <FarmerEd[m]> | I'll need to go study the farmOS docs........... |
| [02:10:46] | <FarmerEd[m]> | But I've mange authentication with Google and ICBF (Irish Cattle Breeders Federation) which both use "Authorization Code" as grant type to retrieve access /refresh tokens. |
| [02:23:44] | <paul121[m]> | Oh great! Well we should support it as well. Would be great if you can test đ |
| [04:35:17] | * farmBOT has joined #farmos |
| [06:07:23] | <FarmerEd[m]> | I can over the weekend, I presume if I build a flow that works with the current version it should work with the new version? (Or at least that's the plan when you guys sort that end out) |
| [07:31:47] | <mstenta[m]> | > But.. otherwise this is kinda a breaking change with no upgrade path :-/ |
| [07:31:47] | <mstenta[m]> | Huh yea that's not cool |
| [07:39:16] | <mstenta[m]> | Farmer Ed: worth noting, the removal of the password grant flow is in the `6.x` branch of the `simple_oauth` module... not the `5.2.0` release that we are working on upgrading to. So there's still time to figure that out... I get the sense that `6.x` changes a number of things, so it will require a thorough review. |
| [07:41:36] | <mstenta[m]> | > Field kit should really be using the authorization flow as well. |
| [07:41:36] | <mstenta[m]> | What confuses me about this is... think about every mobile app you use that requires a login. They ALL require username/password... right? Twitter... Gmail... are those not using password flow? |
| [07:42:06] | <mstenta[m]> | Isn't this the difference between "first party" and "third party"? |
| [07:42:55] | <FarmerEd[m]> | Ah ok... |
| [07:42:55] | <FarmerEd[m]> | But probably still worth experimenting other grant types anyway. |
| [07:43:35] | <mstenta[m]> | Yea not a bad idea to prepare... and I agree for node red it would probably make more sense to use client credentials grant |
| [07:44:08] | <mstenta[m]> | Since it's sort of an "unattended process on a server somewhere" - safer to use something different than normal username/password |
| [07:45:38] | <mstenta[m]> | (ideally... I mean obviously we're not really dealing with life or death safety situations here haha) |
| [08:06:24] | <mstenta[m]> | paul121: this also jumps out at me from https://www.drupal.org/project/simple_oauth/issues/3261247 |
| [08:06:24] | <mstenta[m]> | > This would include things like requiring PKCE, for instance. |
| [08:07:18] | <FarmerEd[m]> | Google doesn't use the password grant flow, the app redirects the user to Googles single sign on page and after the user authenticates with username/password sends the access tokens to the redirect URI after which the app continues to operate using access and refresh tokens. |
| [08:07:47] | <mstenta[m]> | Not sure if that change has been made, or what it means exactly... I just learned of PKCE recently in the USDA work (not much detail.... just that the USDA eAuth identity server normally requires PKCE, but the OpenID Connect module we were using didn't use it, so they disabled PKCE for us specifically...) |
| [08:08:41] | <mstenta[m]> | Farmer Ed: Ah ok that's right... I forgot. I learned that recently (the Google Play Services app on Android basically handles all the SSO for other Google apps). |
| [08:09:21] | <mstenta[m]> | I switched to GrapheneOS recently in a step towards ridding myself of the invasive Google Play Services đ |
| [08:09:51] | <mstenta[m]> | I wonder if other apps (eg: Twitter) are still using password flows, but are going to eventually move away from that too? |
| [08:10:07] | <mstenta[m]> | And just redirect to a web page to initiate auth flow? |
| [08:11:21] | <mstenta[m]> | Well this will certainly add *another* hurdle to Field Kit 2.x development, unless we add our own password grant flow plugin (cc jgaehring ) |
| [08:12:06] | <mstenta[m]> | symbioquine: what does asset link do? Or is that just embedded in the farmOS instance, and so gets authentication "for free" by virtue of being logged in already? |
| [08:13:32] | <FarmerEd[m]> | Or something to that effect, actually got the whole sequence working in Node-Red. |
| [08:17:28] | <FarmerEd[m]> | https://farmer-eds-shed.com/interacting-with-google-calendar-using-node-... |
| [08:19:16] | <mstenta[m]> | paul121: I commented on that issue https://www.drupal.org/project/simple_oauth/issues/3277730#comment-14714525 |
| [08:58:04] | <jgaehring[m]> | <mstenta[m]> "Well this will certainly add *..." <- yea, I always wondered if we would need to adopt the authorization code grant flow, with a redirect to the farmOS server... what was the original issue that raised this here, in a nutshell? |
| [08:58:45] | <mstenta[m]> | yea, sounds like password grant is removed entirely from the oauth2 module 6.x branch. we are not updating to that soon, but a future consideration... |
| [09:08:29] | <jgaehring[m]> | I just noticed the official OAuth pages list it as "Legacy" |
| [09:12:51] | <mstenta[m]> | > I also feel pretty confident that this won't affect current API/OAuth integrations |
| [09:12:51] | <mstenta[m]> | paul121 Meant to say: very glad to hear this! đ |
| [09:13:45] | <mstenta[m]> | Do you think we can merge this `2.x-simple_oauth` into `2.x`? Is there other testing we might want to do? |
| [09:52:55] | <paul121[m]> | > <@mstenta:matrix.org> paul121: this also jumps out at me from https://www.drupal.org/project/simple_oauth/issues/3261247 |
| [09:52:55] | <paul121[m]> | > |
| [09:52:55] | <paul121[m]> | > > This would include things like requiring PKCE, for instance. |
| [09:52:55] | <paul121[m]> | Oh yes thanks. I meant to link to this issue |
| [09:55:39] | <paul121[m]> | The first part/third party thing is kinda a cover up for not implementing the better option anyways :P |
| [09:56:42] | <paul121[m]> | <mstenta[m]> "Do you think we can merge this `..." <- I think it should be fine. But maybe we should add some tests that make sure things work for a client without a secret configured |
| [10:00:49] | <mstenta[m]> | > The first part/third party thing is kinda a cover up for not implementing the better option anyways :P |
| [10:00:49] | <mstenta[m]> | Oh ok gotcha |
| [10:00:58] | <mstenta[m]> | This stuff is changing faster than I can learn it :-) |
| [10:05:43] | <mstenta[m]> | paul121: Does Rothamsted use password flow in their python script? Do you know? |
| [10:07:56] | <paul121[m]> | I believe so. I believe we only support password flow in farmOS.py (possible to use other grants but have to bring your own code for that) |
| [10:09:04] | <mstenta[m]> | OK so yea... farmOS.py would break :-( |
| [10:09:32] | <mstenta[m]> | Does farmOS.js use password flow too? Among others I assume? |
| [10:09:57] | <paul121[m]> | Lately though, some CLI clients have started using the authorization flow. I'm not sure how it all works, the terminal will open a browser and redirect back to the terminal and authenticate without any input from user (no copy/paste tokens, etc) |
| [10:10:54] | <mstenta[m]> | hmm - but then you can't run via cron? |
| [10:10:54] | <paul121[m]> | Yeah I think it is password only |
| [10:10:55] | <paul121[m]> | Correct |
| [10:10:59] | <mstenta[m]> | what is the recommended approach for cron? |
| [10:11:08] | <paul121[m]> | farmOS.py does let you bring your own token though - that's how that aggregator works |
| [10:11:13] | <paul121[m]> | Probably client credentials |
| [10:11:17] | <mstenta[m]> | gotcha |
| [10:11:23] | <mstenta[m]> | ok |
| [10:11:28] | <mstenta[m]> | makes sense |
| [11:49:03] | <mstenta[m]> | I'm testing the github actions stuff for pushing to docker hub... getting there!! successfully pushing `2.x` and tags... just refining some of the details now |
| [11:49:17] | <mstenta[m]> | the `latest` tag logic isn't working yet... |
| [11:49:45] | <mstenta[m]> | https://hub.docker.com/repository/docker/mstenta/farmos/tags?page=1&orde... |
| [11:50:17] | <mstenta[m]> | `if [[ ${{ env.FARMOS_VERSION }} =~ ^(?!0\d)\d+(?:\.(?!0\d)\d+){2}(?:-(?:(?!0\d)(?:(?!_)[\w-])+|\.(?!\.|$))+)?(?:\+(?:(?:(?!_)[\w-])+|\.(?!\.|$))+)?$ ]]; then` |
| [11:50:33] | <mstenta[m]> | ^ that regex doesn't seem to be working as i had hoped... just need to figure that out |
| [11:50:41] | <mstenta[m]> | (from https://github.com/semver/semver/issues/199#issuecomment-43640395) |
| [12:21:53] | <evered[m]> | do you all get paid to work on farmOS ? |
| [12:21:53] | <evered[m]> | I know there are scholarships with GitHub paid work performed. |
| [12:21:53] | <evered[m]> | I am excited to hear about "docker hub". Thank you! Blessings. |
| [12:22:29] | <mstenta[m]> | it's a mix of volunteer and sponsored work for specific features etc |
| [12:23:26] | <mstenta[m]> | yea! we've been using docker hub forever, but we're moving the build process to github actions (which then pushed to docker hub) - slight difference but it will give us more control AND let us start building for other architectures as well like Raspberry Pi :-) |
| [12:33:49] | <evered[m]> | Yay! I like small systems like the Raspberry Pi.... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/f2b914f430...) |
| [12:34:32] | <mstenta[m]> | That's great evered! All help is welcome! It's great to have you! |
| [12:48:51] | <mstenta[m]> | Doh! For some reason our tests stopped passing... interrupting my Docker Hub testing :-( |
| [12:49:16] | <mstenta[m]> | `pgsql` test failure:... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/01a9974b84...) |
| [12:53:01] | <symbioquine[m]> | Maybe a real regression? |
| [12:53:48] | <mstenta[m]> | Yea maybe |
| [12:53:59] | <mstenta[m]> | Nothing has been changes in the code since beta7 which passed |
| [12:54:04] | <mstenta[m]> | So maybe an upstream dependency change |
| [12:54:24] | <mstenta[m]> | Not `consumers` module or `simple_oauth` related... we are pinning both now |
| [13:00:43] | <mstenta[m]> | On a different note... I'm not sure it's going to be easy to check semver regex via bash :-/ |
| [13:00:58] | <mstenta[m]> | The official semver regex patterns don't work in bash |
| [13:00:58] | <mstenta[m]> | https://semver.org/#is-there-a-suggested-regular-expression-regex-to-che... |
| [13:01:40] | <mstenta[m]> | so might need to rethink how we tag `latest`... |
| [13:02:02] | <mstenta[m]> | or make a simpler regex that works with bash, and would fit most of our needs |
| [13:02:19] | <symbioquine[m]> | Grep and Sed both support perl regex though right? |
| [13:02:34] | <mstenta[m]> | oh! you know more than I do apparently đ
|
| [13:02:58] | <symbioquine[m]> | Hmmm, just enough to get myself in trouble |
| [13:02:58] | <mstenta[m]> | haha |
| [13:03:04] | <symbioquine[m]> | What are you trying to do with that regex from the semver page? |
| [13:03:17] | <mstenta[m]> | this is the one i'd like to use: `^(?P<major>0|[1-9]\d*)\.(?P<minor>0|[1-9]\d*)\.(?P<patch>0|[1-9]\d*)(?:-(?P<prerelease>(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+(?P<buildmetadata>[0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$` |
| [13:03:30] | <mstenta[m]> | i just want to check if `2.0.0` matches that basically |
| [13:03:33] | <symbioquine[m]> | Just see if a given tag is a semantic version string? |
| [13:03:39] | <mstenta[m]> | (and i'll replace `2.0.0` with the `FARMOS_VERSION` in the github action) |
| [13:04:40] | <symbioquine[m]> | Cool, I think we should be able to do something like `echo '2.0.0' | grep -Pq '^(?P<major>0|[1-9]\d*)\.(?P<minor>0|[1-9]\d*)\.(?P<patch>0|[1-9]\d*)(?:-(?P<prerelease>(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+(?P<buildmetadata>[0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$' && echo 'yes' || echo 'no'` |
| [13:05:01] | <symbioquine[m]> | (^ not tested) |
| [13:05:43] | <symbioquine[m]> | Seems like it works |
| [13:06:46] | <mstenta[m]> | thank you!!! |
| [13:07:04] | <mstenta[m]> | yea that works great!! |
| [13:07:05] | <symbioquine[m]> | ACTION sent a sh code block: https://libera.ems.host/_matrix/media/r0/download/libera.chat/c9e508a220... |
| [13:07:19] | <mstenta[m]> | beautiful! good enough for me! |
| [13:07:51] | <symbioquine[m]> | Glad I could help! |
| [13:20:31] | <mstenta[m]> | ok novice question... how would I use this in `if [[ ... ]] then;` đ¤ |
| [13:20:46] | <mstenta[m]> | (or maybe i need to restructure my logic) |
| [13:21:33] | <mstenta[m]> | if [[ ... ]]; then |
| [13:21:33] | <mstenta[m]> | docker tag farmos/farmos:2.x farmos/farmos:latest |
| [13:21:33] | <mstenta[m]> | docker push famros/farmos:latest |
| [13:21:33] | <mstenta[m]> | fi |
| [13:24:58] | <symbioquine[m]> | ACTION sent a sh code block: https://libera.ems.host/_matrix/media/r0/download/libera.chat/8a6d82bec4... |
| [13:25:13] | <mstenta[m]> | oh simple! |
| [13:26:09] | <symbioquine[m]> | Yeah, I always have to look up the bash conditional/loop syntax, but once I get it working I'm always like "that's simple, I can remember that..." |
| [13:26:19] | <mstenta[m]> | haha yea |
| [13:27:27] | <mstenta[m]> | alright running a test on this logic to see if it tags `latest`... |
| [13:27:47] | <mstenta[m]> | in the meantime i'll look into this regression (it is definitely a regression, i just reran the tests on 2.0.0-beta7 and they failed in the same way) |
| [13:31:53] | <symbioquine[m]> | Pure speculation here, but it does seem like the sort of thing that might break subtly if PHP or Symphony changed how they handle headers as part of the version updates. |
| [13:32:26] | <mstenta[m]> | It's passing locally (but I haven't rebuilt the codebase) |
| [13:35:29] | <mstenta[m]> | Yea your speculation might be correct symbioquine |
| [13:36:07] | <mstenta[m]> | This is what's failing: https://github.com/farmOS/farmOS/blob/8a2dd9474b4b24cf0cad9cb751edd83405... |
| [13:36:14] | <mstenta[m]> | Not finding all the headers it's expecting |
| [13:39:33] | <mstenta[m]> | > alright running a test on this logic to see if it tags `latest`... |
| [13:39:33] | <mstenta[m]> | Well good news is this worked! |
| [13:48:11] | <mstenta[m]> | > This is what's failing: https://github.com/farmOS/farmOS/blob/8a2dd9474b4b24cf0cad9cb751edd83405... |
| [13:48:11] | <mstenta[m]> | Oh specifically it seems to be the `Access-Control-Allow-Credentials` header that is being set to `false` when it should be `true`... hmmmm |
| [13:53:27] | <mstenta[m]> | Hmm a new version of `symfony/http-foundation` was released 10 hours ago... đ¤đ§ |
| [13:53:28] | <mstenta[m]> | https://github.com/symfony/http-foundation/releases/tag/v4.4.46 |
| [13:53:41] | <mstenta[m]> | And presumably other `symfony/*` libs... |
| [13:55:27] | <mstenta[m]> | Hmm https://github.com/symfony/symfony/issues/47747 |
| [13:55:40] | <mstenta[m]> | Opened 4 hours ago... seems related |
| [13:58:43] | <mstenta[m]> | Oh maybe this: https://github.com/symfony/symfony/pull/47530 |
| [13:59:16] | <mstenta[m]> | err maybe not... |
| [16:07:51] | <mstenta[m]> | Just tested `composer update symfony/http-foundation` (which updated from v4.4.45 to v4.4.46) and re-running my tests locally and now they fail. So this is confirmation that it was a change in `http-foundation` that caused this. âšī¸ |
| [16:17:54] | <mstenta[m]> | > > This is what's failing: https://github.com/farmOS/farmOS/blob/8a2dd9474b4b24cf0cad9cb751edd83405... |
| [16:17:54] | <mstenta[m]> | > |
| [16:17:54] | <mstenta[m]> | > Oh specifically it seems to be the `Access-Control-Allow-Credentials` header that is being set to `false` when it should be `true`... hmmmm |
| [16:17:54] | <mstenta[m]> | Oooh wait! I was mistaken! |
| [16:18:16] | <mstenta[m]> | Stepping through with my debugger now... it's not the `Access-Control-Allow-Credentials` that is causing this... it's `Vary`! |
| [16:18:50] | <mstenta[m]> | I bet something changed in `http-foundation` that causes `Vary` to be added, when it wasn't before... and our test is just not designed with that in mind. |
| [16:20:41] | <mstenta[m]> | Yess... `Vary: Accept-Encoding` is set, and it wasn't before |
| [16:53:04] | <mstenta[m]> | https://github.com/mstenta/farmOS/commit/7694b2cbd3ee02884ddceba3f8e0bae... |