Skip to content

adfs2019 + new lab api#138

Merged
SomkaPe merged 5 commits intodevfrom
pesomka/adfs2019
Dec 6, 2019
Merged

adfs2019 + new lab api#138
SomkaPe merged 5 commits intodevfrom
pesomka/adfs2019

Conversation

@SomkaPe
Copy link
Copy Markdown
Contributor

@SomkaPe SomkaPe commented Dec 5, 2019 •
edited
Loading

  • ADFS2019 support
  • integration test switched to new Lab api

few issues found with new lab api:

  1. "localhost" redirect URL is not configured for B2C applications, so corresponding tests (B2C auth code flow) are disabled
  1. upn of guest user returned from lab contains character '#' , so user discovery request
    fails(because http GET is used),
    I have tried to "url encode" upn, but "userrealm" endpoint fails to identify a user
    it was fixed - homeUpn should be used for guest users.

Going to discuss mentioned issued with lab team

sangonzal
sangonzal reviewed Dec 5, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@sangonzal sangonzal left a comment

Choose a reason for hiding this comment

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

Looks great. Much cleaner.

A couple of notes:

  1. The tests should also work on Azure DevOps. Can you make sure that everything works there?
  2. There are a couple of unit tests that (I believe they are under AuthorityTests, but there might be others) that had been commented out until we added support for ADFS. Consider uncommenting those and making sure that they pass.
  3. The few TODOs that you have pointed out we are still waiting for from the lab

@SomkaPe
Copy link
Copy Markdown
Contributor Author

SomkaPe commented Dec 6, 2019

Looks great. Much cleaner.

A couple of notes:

  1. The tests should also work on Azure DevOps. Can you make sure that everything works there?
  2. There are a couple of unit tests that (I believe they are under AuthorityTests, but there might be others) that had been commented out until we added support for ADFS. Consider uncommenting those and making sure that they pass.

good catch about some disabled adfs unit test 👍
test on DevOps are green, there was minor issue with chromium driver version

@SomkaPe SomkaPe merged commit d2a74ef into dev Dec 6, 2019
This was referenced Dec 17, 2019
Closed
Closed
@sangonzal sangonzal deleted the pesomka/adfs2019 branch January 22, 2020 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@henrik-me henrik-me Awaiting requested review from henrik-me

@navyasric navyasric Awaiting requested review from navyasric

1 more reviewer

@sangonzal sangonzal sangonzal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@SomkaPe @sangonzal