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

Add new provider - SAML via Azure AD #169

Closed
wants to merge 1 commit into from
Closed

Add new provider - SAML via Azure AD #169

wants to merge 1 commit into from

Conversation

greenpau
Copy link

Resolves: #168

@greenpau greenpau changed the title Add new provider - SAML via Azure AD WIP: Add new provider - SAML via Azure AD Feb 16, 2020
@coveralls
Copy link

coveralls commented Feb 16, 2020 •
edited

Coverage Status

Coverage decreased (-7.3%) to 84.116% when pulling 41b7183 on greenpau:samlprovider into b759a6a on tarent:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.423% when pulling b6c0685 on greenpau:samlprovider into b759a6a on tarent:master.

@greenpau greenpau force-pushed the samlprovider branch 8 times, most recently from 3acaba5 to 1a72c1d Compare February 17, 2020 03:28
@greenpau
Copy link
Author

@smancke , qq, I create a doc for SAML provider here.

Should I put SAML provider documentation in a separate file or should I keep it in the main README.md?

I am documenting the work first, then I will put the necessary code together.

@greenpau greenpau force-pushed the samlprovider branch 2 times, most recently from cee0b9a to 073aeec Compare February 17, 2020 03:44
@g-w
Copy link
Contributor

g-w commented Feb 19, 2020

Hello @greenpau,

thank you for your contribution!

For now simply put the documentation in the README.md.

@greenpau greenpau force-pushed the samlprovider branch 11 times, most recently from fb49bf7 to ba80425 Compare February 24, 2020 12:58
@greenpau greenpau requested a review from g-w February 24, 2020 23:19
@greenpau
Copy link
Author

@g-w , could you please review the addition. I know that the coverage is not there. I would like to get general agreement on the proposal. Once finalized, I will improve the coverage.

@greenpau greenpau changed the title WIP: Add new provider - SAML via Azure AD Add new provider - SAML via Azure AD Feb 27, 2020
@greenpau
Copy link
Author

greenpau commented Mar 9, 2020

@g-w , any feedback?

@g-w
Copy link
Contributor

g-w commented Mar 9, 2020

Hello @greenpau,

sorry for the delays, but since we are currently not using azure and i am not familiar with it, hence reviewien the changes is a bigger task. I will come back to in the next days.

g-w
g-w suggested changes Mar 16, 2020
View reviewed changes
Copy link
Contributor

@g-w g-w left a comment

Choose a reason for hiding this comment

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

Hello @greenpau,

I had a look at your changes. Currently there are some problems with your changes that have to be fixed.

  • The functionality should work without caddy
  • Azure SAML should work like the other authentication backends, ie should follow the configuration style
  • There are (almost) no tests that assure the correctness of the implementation

Please have a look into httpasswd backend for an example how the backend mechanism works. If you have any further questions, please come back to me.

README.md
login {
success_url /private

azure_enabled true
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the configuration scheme as the other methods. It should read something like:

azure_saml metadata_location=/... idp_sign_cert_location=/...

README.md

### Azure AD Applications

#### Caddy Configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put documentation on caddy specific Configuration in /caddy/README.md and loginsrv standalone configuration in the /README.md.

README.md

#### Set Up Azure AD Application

In Azure AD, you will have an application, e.g. "My Infosec".
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this section should be shorter and assume general knowledge on azure saml.

login/handler.go
if config.Azure == nil {
return nil, errors.New(errMsg)
}
if !config.Azure.Enabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

Azure SAML should be a login backend.

@greenpau
Copy link
Author

greenpau commented May 9, 2020

moved it to a new caddy v2 module https://github.com/greenpau/caddy-auth-forms

@greenpau greenpau closed this May 9, 2020
@greenpau greenpau deleted the samlprovider branch May 9, 2020 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@g-w g-w g-w requested changes

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

Successfully merging this pull request may close these issues.

Add new provider - SAML via Azure AD
3 participants
@greenpau @coveralls @g-w