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.

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

Give warning to mandatory MongoDB attributes properties 6.1.x #4404

Closed
wants to merge 1 commit into from

Conversation

NgSekLong
Copy link
Contributor

@NgSekLong NgSekLong commented Nov 4, 2019

See discussion: https://groups.google.com/a/apereo.org/forum/#!topic/cas-user/_aOhb33YZXo


Since defining attributes is necessary for pac4j to work when using MongoDB Authentication, the attributes properties is necessary here.

However, this behavior of requiring attributes is different from other authentication methods (e.g.JDBC), so I proposed to add an warning here for clarity sake. See if agree.

Would port back to master once this is merged.

See discussion: https://groups.google.com/a/apereo.org/forum/#!topic/cas-user/_aOhb33YZXo
--------------
Since defining `attributes` is necessary for pac4j to work when using MongoDB Authentication, the `attributes` properties is necessary here.

However, this behavior of requiring `attributes` is different from other authentication methods, so I proposed to add an warning here for clarity sake. See if agree.

Would port back to master once this is merged.
@mmoayyed
Copy link
Member

mmoayyed commented Nov 4, 2019

Since defining attributes is necessary for pac4j to work when using MongoDB Authentication, the attributes properties is necessary here. However, this behavior of requiring attributes is different from other authentication methods (e.g.JDBC), so I proposed to add an warning here for clarity sake. See if agree.

Thanks for the patch but none of this sounds right.

Attribute support is always optional. All authentication methods in CAS work with or without presence of attributes in the authentication source. A design choice or limitation of a library should not have to contradict consistent behavior elsewhere. Changes need to be done on pac4j to allow attribute-less authentication.

@mmoayyed
Copy link
Member

mmoayyed commented Nov 4, 2019

I suggest you report the issue to the pac4j project and follow up with a pull request to accommodate this behavior.

@NgSekLong
Copy link
Contributor Author

NgSekLong commented Nov 4, 2019

Good
I was quite confused at first as too way this was considered intended behavior instead of a bug when discussing in the Google Group.

Let me relay this issue to pac4j, thanks.

@leleuj
Copy link
Contributor

leleuj commented Nov 4, 2019

pac4j has its own consistency and it conflicts with the consistency of CAS, but that does not mean we should change pac4j.

My feeling is that the default behavior of pac4j should be kept, but changed when used in CAS to have consistency in both systems (alone or bundle).

Here is my proposal: by default, in CAS, the id,username,password attributes are defined, which makes things consistent in CAS: no need to define the attributes, consistent in pac4j and things will work properly. What do you think?

@mmoayyed
Copy link
Member

mmoayyed commented Nov 4, 2019

Here is my proposal: by default, in CAS, the id,username,password attributes are defined, which makes things consistent in CAS: no need to define the attributes, consistent in pac4j and things will work properly. What do you think?

Yes, that's great!

@NgSekLong NgSekLong deleted the patch-8 branch November 5, 2019 01:08
@NgSekLong
Copy link
Contributor Author

NgSekLong commented Nov 5, 2019

Here is my proposal: by default, in CAS, the id,username,password attributes are defined, which makes things consistent in CAS: no need to define the attributes, consistent in pac4j and things will work properly. What do you think?

I think that will be the best

@leleuj
Copy link
Contributor

leleuj commented Nov 5, 2019

Perfect. I'll submit a PR unless someone is quicker...

@dmaciaszek
Copy link
Contributor

Is there possibility to read serialized profile as previous versions? I think now CAS use only legacy mode of pac4j and is not compatible when someone use catalogue od users with serializedprofile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment