Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: OIDC Broker and EntraID specific group management #13

Merged
merged 11 commits into from
Jun 4, 2024
Merged

feat: OIDC Broker and EntraID specific group management #13

merged 11 commits into from
Jun 4, 2024

Conversation

denisonbarbosa
Copy link
Member

@denisonbarbosa denisonbarbosa commented Apr 10, 2024 •
edited
Loading

I'm opening this as a single draft PR for now so we can have this code listed somewhere. We can decide later whether we should merge this as a single pull request or break it into multiple ones.

UDENG-2039
UDENG-2040
UDENG-2041
UDENG-2042
UDENG-2043

@denisonbarbosa denisonbarbosa force-pushed the impl-broker-package branch 2 times, most recently from 23578b2 to 0e930ab Compare April 15, 2024 12:10
@denisonbarbosa denisonbarbosa force-pushed the impl-broker-package branch 2 times, most recently from f75e105 to 2363708 Compare May 16, 2024 13:50
@denisonbarbosa denisonbarbosa marked this pull request as ready for review May 16, 2024 14:01
@denisonbarbosa denisonbarbosa requested a review from a team as a code owner May 16, 2024 14:01
GabrielNagy
GabrielNagy reviewed May 22, 2024
View reviewed changes
Copy link

@GabrielNagy GabrielNagy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing work, I had some comments, all minor though :)

config/oidc-broker.broker Outdated Show resolved Hide resolved
internal/testutils/provider.go Show resolved Hide resolved
internal/providers/microsoft_entra_id/microsoft-entra-id.go Outdated Show resolved Hide resolved
internal/providers/microsoft_entra_id/microsoft-entra-id.go Outdated Show resolved Hide resolved
internal/broker/broker.go Outdated Show resolved Hide resolved
internal/broker/broker_test.go Outdated Show resolved Hide resolved
didrocks
didrocks requested changes May 24, 2024
View reviewed changes
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven’t done a full review yet, but I think we have an excellent start here!

I think there is more still to review and achieve, but let’s fix those first, and see what the consequences on the code are.

Excellent work!

internal/broker/broker.go Outdated Show resolved Hide resolved
internal/brokerservice/brokerservice.go Show resolved Hide resolved
internal/broker/broker.go Outdated Show resolved Hide resolved
internal/broker/broker.go Outdated Show resolved Hide resolved
internal/broker/helper_test.go Outdated Show resolved Hide resolved
internal/broker/broker.go Outdated Show resolved Hide resolved
internal/broker/broker.go Show resolved Hide resolved
internal/broker/broker_test.go Outdated Show resolved Hide resolved
internal/broker/broker.go Show resolved Hide resolved
internal/broker/broker.go Show resolved Hide resolved
@denisonbarbosa denisonbarbosa force-pushed the impl-broker-package branch 3 times, most recently from f3d3527 to 3f692ce Compare May 28, 2024 16:16
didrocks
didrocks requested changes May 29, 2024
View reviewed changes
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while doing pairing :)

internal/broker/broker.go Outdated Show resolved Hide resolved
internal/broker/broker.go Outdated Show resolved Hide resolved
internal/providers/microsoft_entra_id/microsoft-entra-id.go Outdated Show resolved Hide resolved
internal/broker/broker.go Outdated Show resolved Hide resolved
internal/providers/microsoft_entra_id/microsoft-entra-id.go Outdated Show resolved Hide resolved
internal/broker/broker.go Outdated Show resolved Hide resolved
internal/broker/broker.go Outdated Show resolved Hide resolved
internal/broker/broker.go Show resolved Hide resolved
internal/broker/broker.go Outdated Show resolved Hide resolved
internal/broker/broker.go Outdated Show resolved Hide resolved
didrocks
didrocks reviewed Jun 3, 2024
View reviewed changes
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go.sum conflict fix + need some feedbacks on the investigation on file touching (and a test)

@denisonbarbosa
Implement missing methods on the service
6890ad5 
There were some missing dbus methods in brokerservice (and the
NewSession method definition was also missing the input argument for the
session mode)
@didrocks
Copy link
Member

didrocks commented Jun 4, 2024

I was expecting to see some tests around expiration changing, do we miss any?

I think a test with offline login and below offline cache expiration date, we can login and another one where we are after that date, and can’t login.

We had similar tests in aad-auth.

@denisonbarbosa
Copy link
Member Author

denisonbarbosa commented Jun 4, 2024 •
edited
Loading

I was expecting to see some tests around expiration changing, do we miss any?

I think a test with offline login and below offline cache expiration date, we can login and another one where we are after that date, and can’t login.

We had similar tests in aad-auth.

Like these?

  1. Offline authentication
  2. Deny authentication if token is offline expired

@didrocks
Copy link
Member

didrocks commented Jun 4, 2024

Like these?

  1. Offline authentication
  2. Deny authentication if token is offline expired

Right, but why weren’t they modified with the latest commit? They should have been impacted or it means, we have a missing test somewhere :)

didrocks
didrocks approved these changes Jun 4, 2024
View reviewed changes
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as per https://warthogs.atlassian.net/browse/UDENG-3078

@denisonbarbosa
Parse the config file when creating the broker
7087102 
We need to check the configuration file in order to gather required
values for the broker creation. This is best done when creating the
brokerservice.
@denisonbarbosa
Rename brokerservice pkg to dbusservice
66cbbc6
@denisonbarbosa
Add mock for an oidc provider
8f69c27
@denisonbarbosa
Update cmd tests with brokerservice changes
a615bf3
@denisonbarbosa
Refactor provider logic used by the broker
1d3f4c1 
Instead of having the providers handled by the internal/broker package,
they now have their own separate package to manage them and their
respective functionalities. This helps a lot when mocking them for the
tests and also make the code scale better.
@denisonbarbosa
Implement EntraID user groups logic
cad35ac
@denisonbarbosa
Add already-known test utilities
cdfa7bc 
Common test utilities such as LoadAndUpdateFromGolden
@denisonbarbosa
Implementation of the OIDC broker
e2f9a1f
@denisonbarbosa denisonbarbosa merged commit 5d9c50f into main Jun 4, 2024
3 checks passed
@denisonbarbosa denisonbarbosa deleted the impl-broker-package branch June 4, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@didrocks didrocks didrocks approved these changes

@GabrielNagy GabrielNagy Awaiting requested review from GabrielNagy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@denisonbarbosa @didrocks @GabrielNagy