Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mcKsf-0004Wh-54 for pgadmin-hackers@arkaria.postgresql.org; Mon, 18 Oct 2021 05:08:13 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1mcKse-0008K2-0X for pgadmin-hackers@arkaria.postgresql.org; Mon, 18 Oct 2021 05:08:12 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mcKsd-0008JX-EJ for pgadmin-hackers@lists.postgresql.org; Mon, 18 Oct 2021 05:08:11 +0000 Received: from mail-lf1-x130.google.com ([2a00:1450:4864:20::130]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1mcKsb-00019i-2B for pgadmin-hackers@postgresql.org; Mon, 18 Oct 2021 05:08:10 +0000 Received: by mail-lf1-x130.google.com with SMTP id z11so63450967lfj.4 for ; Sun, 17 Oct 2021 22:08:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=b2jszAwJyj3IY/KrsuOOzcHptguWnvM8UK14jtMWSSk=; b=AhhlYOaqeJxEhmSPfTvU8EcEvy1kW8EuGLhOeR1FD4TCMAMdwnsxlq9U9Ij4p9S8q5 vFJ0tlsNkIGAiYKPFznXuIrfZ7PDLrOlQq1XzShTLUhoK7cOK1KVsXFuB5cj9gNs1xi/ eycZwpj633KhQUf3PZ1v64bAMmIk9V9kzUmt+xjSHgsXnXmxxEH8hUQboP41iiiRkWce wbuI79Z8rVzoYBFR2xpBE49XEsbfv+sqrebVW1+D+7LmCXAyooRBx4+064orEqBLLO9R GkD3PsP/2R+MPv4EUs67rhsTAOtp0ncIMCKOB0472FGy9MR77LNpywlOYKkepQe/NLa+ gRVA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=b2jszAwJyj3IY/KrsuOOzcHptguWnvM8UK14jtMWSSk=; b=AVwaz8eahC2JqBlt7e0KCFBL25WV3FSwvf4w3ISMneDOWsUK5wWZRLVUhjUzAVFvZX x0gaxLTJNPiS7cAmPooJd2Ax++GQ/ka+BQtaVaedrbnsRgCQ3yh1b/lPTH1mTnAFBToI eXtCt/qxb3WLyWaZfhTqoV8ly5wzTeUN3cOK1eKJ2gBEhyh6dUnTTeDrYdOwwld6tYnR 5n76VPTX13ZpBSDukN639fgUsx1eZ9nkaGaYvDS8QZfy6kAVI2F1Rs0Tj0VdlbpWXZV2 GUv57LH0eemJgJERGSSVRQFR4A5vUCpfusEknwAP7z9HGp9xv/hnYDzeG4WlNKw2ch9q IJkA== X-Gm-Message-State: AOAM531q4Fqkf3uoN/bEMupsGyYfrhEXoSNJuawDT2heXMuegd3OqZSW vu1pOCE+lERKWUj668PgeyhR8yJA6diss3wE9YLRfumD2CJl4hWw1viDXs2nEdtzFQfeFAzAh6z fT7URHU3F8oGjcpHB/46EiRBNOXYiPmxSIds7+0IgD5FqAxy1iCBCIpZwdJ3rmuup0kZFyQurb5 w2+/tiKduZyy7U/RiFUSHU8JHfCj4fEzXfUSpGb/IjWdj+JTmNQ6eLyyUwRw== X-Google-Smtp-Source: ABdhPJykVbM22+pavvWthMsRmWkwyTTHZarEV51tbW1SmTwNkSXkeLuJ0lfY/Z1+7dnelXF/L4NzadfRjAMy5xJ4yxM= X-Received: by 2002:ac2:4bc1:: with SMTP id o1mr26416629lfq.520.1634533686495; Sun, 17 Oct 2021 22:08:06 -0700 (PDT) MIME-Version: 1.0 References: <1d3c17a7-1721-ab4c-c1ef-d206fcd20870@posteo.de> In-Reply-To: <1d3c17a7-1721-ab4c-c1ef-d206fcd20870@posteo.de> From: Khushboo Vashi Date: Mon, 18 Oct 2021 10:37:56 +0530 Message-ID: Subject: Re: feature #6640 To: Florian Sabonchi Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000c1dee405ce998798" X-CLOUD-SEC-AV-Info: enterprisedb,google_mail,monitor X-CLOUD-SEC-AV-Sent: true X-Gm-Spam: 0 X-Gm-Phishy: 0 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000c1dee405ce998798 Content-Type: text/plain; charset="UTF-8" Hi Florian, Review comments: - Allowed_organisation is introduced for all, so the code comments and documentation should reflect it. Github should be an example of that. - The below code checks all the Oauth2 configs, so if I have set ALLOWED_ORGANIZATIONS for only github, it will check for all the configured oauth2 servers, which will give the wrong result in case of multiple providers/servers. Use the current Oauth2 client, self .oauth2_current_client]['ALLOWED_ORGANIZATION'] instead. for oauth2_config in config.OAUTH2_CONFIG: allowed_organizations = oauth2_config['ALLOWED_ORGANIZATIONS'] - 'ALLOWED_ORGANIZATIONS' should be conditional. if it's in the config, then only go further and check the user's validity, otherwise the current users who are using Oauth2 will face the problem. - The patch doesn't apply on the latest code, please rebase your patch. Thanks, Khushboo On Wed, Oct 13, 2021 at 4:03 PM Florian Sabonchi wrote: > Hi I have written a patch for feature #6640 > > --000000000000c1dee405ce998798 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Florian,

Review comments:

- Allowed_organis= ation is introduced for all, so the code comments and documentation should = reflect it. Github should be an example of that.
- The below code checks all the Oauth2 configs, so = if I have set ALLOWED_ORGANIZATIONS for only github, it will check for all = the configured oauth2 servers, which will give the wrong result in case of = multiple providers/servers. Use the current Oauth2=C2=A0client,=C2=A0self.oauth2_current_client]['ALLOWED_ORG= ANIZATION'] ins= tead.
=C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for oaut= h2_config in config.OAUTH2_CONFIG:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 allowed_organizations =3D oauth2_config['AL= LOWED_ORGANIZATIONS']
- = 9;ALLOWED_ORGANIZATIONS' should be conditional. if it's in the config, then= only go further and check the user's validity, otherwise the current u= sers who are using Oauth2 will face the problem.
- The patch doesn't apply on the latest code, p= lease rebase your patch.
=
Thanks,
=
Khushboo

On Wed, Oct 13, = 2021 at 4:03 PM Florian Sabonchi <= sabonchi@posteo.de> wrote:
Hi I have written a patch for feature #6640

--000000000000c1dee405ce998798--