FLI-1258: pass org and teams to metadata #3654

devumesh · 2024-11-25T15:45:11Z

Approach:

Get orgs and teams of the authenticated user by default. I went with this approach because there might be a case where authentication is not needed based on the orgs or teams but authorization might be needed to control RBAC.
Validate authentication on orgs if orgs > 0.
Validate team if allowed_teams > 0.
Convert orgs to list (stringified) and store in io.flipt.auth.github.orgs.
Convert teams to map {org1: [list of teams],org2: [list of teams]} and store in io.flipt.auth.github.teams.

closes #3435

Signed-off-by: devumesh <[email protected]>

codecov · 2024-11-25T16:00:02Z

Codecov Report

Attention: Patch coverage is 87.35632% with 11 lines in your changes missing coverage. Please review.

Project coverage is 64.41%. Comparing base (70295f8) to head (4d2729c).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/server/authn/method/github/server.go	87.35%	7 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3654      +/-   ##
==========================================
+ Coverage   64.30%   64.41%   +0.10%     
==========================================
  Files         170      170              
  Lines       17031    17099      +68     
==========================================
+ Hits        10952    11014      +62     
- Misses       5396     5400       +4     
- Partials      683      685       +2

Flag	Coverage Δ
unittests	`64.41% <87.35%> (+0.10%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

erka

Thank you for the contribution @devumesh!

There are few things which we should address in this PR.

Original code hides some aspects of Github login integration and tests does not cover it. There should be a token with read:org in scope to fetch user's orgs and teams. Originally the check happens on the configuration level https://github.com/flipt-io/flipt/blob/main/internal/config/authentication.go#L600-L602. Without that getUserOrgs and getUserTeamsByOrg api requests will fail and auth flow will be broken for those who don't have that scope in their Flipt configuration.

Also I would like to discuss another thing. I think we should not store all user's orgs and teams, if there is a configuration requirement for specific organizations and/or teams for privacy reasons. wdyt? cc: @markphelps

markphelps

thanks @devumesh ! I agree with @erka that we could use a bit more testing.

@erka im not too concerned about storing all the orgs/team memberships in the metadata tbh. how are you thinking we would limit what is stored in metadata? another configuration option?

internal/server/authn/method/github/server.go

markphelps · 2024-11-26T14:42:05Z

internal/server/authn/method/github/server.go

-		if err != nil {
-			return nil, err
-		}
+	userOrgs, err := getUserOrgs(ctx, token, apiURL)


note for another PR: we should consider caching these results in memory for a period of time as now we're doing these REST calls on every request if an org is using GitHub authn and the orgs/team membership do not change nearly that often

erka · 2024-11-26T21:15:13Z

@erka im not too concerned about storing all the orgs/team memberships in the metadata tbh. how are you thinking we would limit what is stored in metadata? another configuration option?

Let say, that Flipt configuration has allowed_organizations which set to [a, b]. And user has orgs [a, d, f, g, e, t]. Is there any usecase when those d, f, g, e, t could be useful? I think that only allowed_organizations should be added to metadata in this case.

markphelps · 2024-11-27T00:00:14Z

@erka im not too concerned about storing all the orgs/team memberships in the metadata tbh. how are you thinking we would limit what is stored in metadata? another configuration option?

Let say, that Flipt configuration has allowed_organizations which set to [a, b]. And user has orgs [a, d, f, g, e, t]. Is there any usecase when those d, f, g, e, t could be useful? I think that only allowed_organizations should be added to metadata in this case.

Ah that's a good point. I was thinking you were suggesting another config property but I agree with you we should only track the ones configured.

devumesh · 2024-11-27T14:35:19Z

Original code hides some aspects of Github login integration and tests does not cover it. There should be a token with read:org in scope to fetch user's orgs and teams. Originally the check happens on the configuration level https://github.com/flipt-io/flipt/blob/main/internal/config/authentication.go#L600-L602. Without that getUserOrgs and getUserTeamsByOrg api requests will fail and auth flow will be broken for those who don't have that scope in their Flipt configuration.

Hi @erka, correct me If I'm understanding wrong.
read:org in scope should me mandated when using Github?

devumesh · 2024-11-27T14:38:23Z

Let say, that Flipt configuration has allowed_organizations which set to [a, b]. And user has orgs [a, d, f, g, e, t]. Is there any usecase when those d, f, g, e, t could be useful? I think that only allowed_organizations should be added to metadata in this case.

I have a doubt. If the user doesn't configure allowed_organizations, should we need to pass [a, d, f, g, e, t] orgs for authz or authn is needed for authz with github?

erka · 2024-11-27T15:08:28Z

Hi @erka, correct me If I'm understanding wrong. read:org in scope should me mandated when using Github?

Maybe if your Flipt instance is in internet. But I don't think we can't change it, at least in v1.

It started with user:email scope. One day someone asked for organization filter because of access control concern. I believe they had the Flipt instance in public network. Later someone else asked for teams support. For these orgs and team filters read:org is needed. The documentation looks like a history in some sense.

It would be great to not break auth flow for those first adopters of login with Github.

erka · 2024-11-27T15:12:01Z

I have a doubt. If the user doesn't configure allowed_organizations, should we need to pass [a, d, f, g, e, t] orgs for authz or authn is needed for authz with github?

Yeah, it's tricky... If there is read:org in configuration scopes , let's read them and add to metadata, if not - we can't fetch them and they should be empty. wdyt?

devumesh · 2024-11-28T04:29:49Z

Yeah, it's tricky... If there is read:org in configuration scopes , let's read them and add to metadata, if not - we can't fetch them and they should be empty. wdyt?

Yeah I guess it should work. If the allowed org is not provided are you suggesting to send all the retrieved orgs for authz?
or
Authz for github with org and teams valid only with allowed orgs for authn?

Can you please clarify this.

Signed-off-by: devumesh <[email protected]>

erka · 2024-11-28T11:47:56Z

@devumesh Let me explain my vision with examples so you could validate them.

1. no read:org in scopes

No way to get user's organizations and teams, so metadata for organizations and teams should be empty

2. read:org in scopes

2.1. no organizations restrictions

Fetch user's orgs and teams and add them all to metadata. For example, user has a, b, e orgs, so metadata will have a, b, e too.

2.2. with organization restrictions

Fetch user's orgs and teams, filter them and include only required organizations which user has. As example, user has a, b, c, d organizations and restriction contains a, b, e. Only a, b orgs and teams of a, b should be included in metadata.

2.3. with organization and team restrictions

Same as 2.2 but filter user's teams as well.

WDYT?

devumesh · 2024-11-28T12:12:33Z

Fetch user's orgs and teams, filter them and include only required organizations which user has. As example, user has a, b, c, d organizations and restriction contains a, b, e. Only a, b orgs and teams of a, b should be included in metadata.

Hey @erka , thanks. I have made changes as you mentioned. But I have made a mistake where instead of passing orgs a, b I'm passing a, b, e (passing all the orgs mentioned in the org restriction). Same mistake I made for teams also. I will fix that.

Signed-off-by: devumesh <[email protected]>

markphelps

lgtm @devumesh !! just one minor style request around instantiating the err var instead of reusing one when doing if err = x; err != nil

internal/server/authn/method/github/server.go

markphelps · 2024-11-29T19:15:34Z

@all-contributors please add @devumesh for code

allcontributors · 2024-11-29T19:15:38Z

@markphelps

@devumesh already contributed before to code

markphelps

Thank you!

Co-authored-by: Mark Phelps <[email protected]>

devumesh · 2024-11-30T02:00:04Z

Hi @markphelps, thank you. committed your suggestion. Please review.

devumesh added 2 commits November 25, 2024 19:57

FLI-1258: pass orgs and teams to metadata

7fe091a

Signed-off-by: devumesh <[email protected]>

FLI-1258: fix unit testcases

2db4799

Signed-off-by: devumesh <[email protected]>

devumesh requested a review from a team as a code owner November 25, 2024 15:45

erka requested changes Nov 25, 2024

View reviewed changes

markphelps requested changes Nov 26, 2024

View reviewed changes

devumesh added 2 commits November 28, 2024 13:01

FLI-1258: handle read:org during callback

fabe368

Signed-off-by: devumesh <[email protected]>

FLI-1258: unit test case to handle allowed orgs and team

ae0a208

Signed-off-by: devumesh <[email protected]>

devumesh added 2 commits November 29, 2024 09:04

FLI-1258: handle the edge case scenario

9900166

Signed-off-by: devumesh <[email protected]>

FLI-1258: test case for edge scenario

1335f72

Signed-off-by: devumesh <[email protected]>

markphelps added the needs docs Requires documentation updates label Nov 29, 2024

markphelps reviewed Nov 29, 2024

View reviewed changes

internal/server/authn/method/github/server.go Outdated Show resolved Hide resolved

markphelps approved these changes Nov 29, 2024

View reviewed changes

FLIP-1258: fix err var reusing

101cae7

Co-authored-by: Mark Phelps <[email protected]>

markphelps added the automerge Used by Kodiak bot to automerge PRs label Nov 30, 2024

Merge branch 'main' into FLI-1258

4d2729c

markphelps merged commit 96baaca into flipt-io:main Nov 30, 2024
36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FLI-1258: pass org and teams to metadata #3654

FLI-1258: pass org and teams to metadata #3654

devumesh commented Nov 25, 2024 •

edited by erka

Loading

codecov bot commented Nov 25, 2024 •

edited

Loading

erka left a comment •

edited

Loading

markphelps left a comment

markphelps Nov 26, 2024

erka commented Nov 26, 2024

markphelps commented Nov 27, 2024

devumesh commented Nov 27, 2024

devumesh commented Nov 27, 2024

erka commented Nov 27, 2024

erka commented Nov 27, 2024

devumesh commented Nov 28, 2024

erka commented Nov 28, 2024

devumesh commented Nov 28, 2024

markphelps left a comment

markphelps commented Nov 29, 2024

allcontributors bot commented Nov 29, 2024

markphelps left a comment

devumesh commented Nov 30, 2024

FLI-1258: pass org and teams to metadata #3654

FLI-1258: pass org and teams to metadata #3654

Conversation

devumesh commented Nov 25, 2024 • edited by erka Loading

codecov bot commented Nov 25, 2024 • edited Loading

Codecov Report

erka left a comment • edited Loading

Choose a reason for hiding this comment

markphelps left a comment

Choose a reason for hiding this comment

markphelps Nov 26, 2024

Choose a reason for hiding this comment

erka commented Nov 26, 2024

markphelps commented Nov 27, 2024

devumesh commented Nov 27, 2024

devumesh commented Nov 27, 2024

erka commented Nov 27, 2024

erka commented Nov 27, 2024

devumesh commented Nov 28, 2024

erka commented Nov 28, 2024

1. no read:org in scopes

2. read:org in scopes

2.1. no organizations restrictions

2.2. with organization restrictions

2.3. with organization and team restrictions

devumesh commented Nov 28, 2024

markphelps left a comment

Choose a reason for hiding this comment

markphelps commented Nov 29, 2024

allcontributors bot commented Nov 29, 2024

markphelps left a comment

Choose a reason for hiding this comment

devumesh commented Nov 30, 2024

devumesh commented Nov 25, 2024 •

edited by erka

Loading

codecov bot commented Nov 25, 2024 •

edited

Loading

erka left a comment •

edited

Loading